refactor: compile Hugr using abstract graph datatype by acl-cqc · Pull Request #97 · Quantinuum/brat

acl-cqc · 2025-12-17T12:41:50Z

Refactor parent outside HugrOp
Make abstract Hugr/Graph type.
- Stores 2-way edges
- separates parent from op (different map), and separates out the root.
- parent-ness is now set when the name is created, not when the op is set (which may be later).
- and may record for each op a list of which children to order first in the output.
Hence, also drop ordering of HugrOp, remove index from Case.

This reverts commit 99a7416.

…ong order??

croyzor · 2026-01-05T11:55:17Z

brat/Data/HugrGraph.hs

+    nameSupply :: Namespace
+} deriving (Eq, Show) -- we probably want a better `show`
+
+splitNamespace :: String -> State HugrGraph Namespace


It might be nice to implement instance FreshMonad (State HugrGraph a) from Brat.Naming

See other comment - have indeed moved namespace out, so the instance FreshMonad would be something else...

croyzor · 2026-01-05T15:11:27Z

brat/Data/HugrGraph.hs

+    _ -> error "addEdge to/from node not present"
+ where
+  addToMap :: Ord k => k -> v -> M.Map k [v] -> M.Map k [v]
+  addToMap k v m = M.insert k (v:(fromMaybe [] $ M.lookup k m)) m


Extra lookup could be avoided with M.alter (though still a bit awkward!)

addToMap k v m = M.alter k (\maybeVs -> Just (v : fromMaybe [] maybeVs)) m

or

addToMap k v m = M.alter k (maybe (Just [v]) (fmap (v:))) m

Got me going in the right direction....M.alter (Just . (v:) . fromMaybe []) k m is succinct and I think reasonably clear

tho it got a bit more complex with the single-incoming-edge check (fromMaybe [] -> maybe [] chk)

croyzor · 2026-01-05T15:19:43Z

brat/Data/HugrGraph.hs

+    nodes :: M.Map NodeId HugrOp,
+    edges_out :: M.Map NodeId [(Int, PortId NodeId)],
+    edges_in :: M.Map NodeId [(PortId NodeId, Int)],
+    nameSupply :: Namespace


Not sure if having the nameSupply here makes sense, it feels like it should be provided by the monad. The monadic functions here could be defined on e.g.:

freshNode :: (MonadState HugrGraph m, FreshMonad m) => NodeId -> String -> m Namespace

but I'm splitting hairs and that smells of premature abstraction

I agree it's better to move the nameSupply out into the Compile monad as that let's up parametrize everything that doesn't use Namespace/create names by the node type - which will be better for #98 among others. So I've done that.

This parametrized freshNode seems pretty complex, but maybe very neat. What instance would I want - instance FreshMonad Compile? instance FrsehMonad (State (HugrGraph NodeId, Namespace)) ?? (I am slightly nervous of this. FreshMonad requires the !- operator to run in a sub-namespace, but when we do that we also want to run on a completely separate hugr so I do kinda feel that running a separate/inner monad is better...)

And would that requirement be MonadState (HugrGraph NodeId, Namespace) m, FreshMonad? Ah - no need to grab the Namespace out of the state, FreshMonad does that work for us? (I have not seen MonadState before, nor the | m -> s syntax in class Monad m => MonadState s (m :: Type -> Type) | m -> s where ....)

If it is just instance (MonadState (HugrGraph NodeId)) Compile....then all the things in HugrGraph.hs that are ... -> State (HugrGraph n/N) Result could be MonadState (HugrGraph n/N) m => ... -> m Result and we do away with onHugr, is that it? (So onHugr was my poor man's way of doing MonadState, but in this case the poor man's way is simple....)

I did try instance (MonadState (HugrGraph NodeId)) Compile but got:

error: [GHC-46208] Functional dependencies conflict between instance declarations: instance MonadState (HugrGraph NodeId) Compile -- Defined at Brat/Compile/Hugr.hs:75:10 instance [safe] Monad m => MonadState s (StateT s m) -- Defined in ‘Control.Monad.State.Class’

I think haskell is confused because Compile, being a state monad, already implements MonadState CompilationState and it doesn't know which one to choose

croyzor · 2026-01-05T15:26:06Z

brat/Data/HugrGraph.hs

+addEdge (src@(Port s o), tgt@(Port t i)) = state $ \h@HugrGraph {..} ->
+  ((), ) $ case (M.lookup s nodes, M.lookup t nodes) of
+    (Just _, Just _) -> h {
+      edges_out = addToMap s (o, tgt) edges_out,


We're not doing anything to avoid doubly-wiring a port, maybe there should be a check here?

Can check the inport yes good plan, checking the output would require understanding the type which feels a bit out of scope for this HugrGraph

croyzor · 2026-01-05T15:32:47Z

brat/Data/HugrGraph.hs

+            })
+
+setFirstChildren :: NodeId -> [NodeId] -> State HugrGraph ()
+setFirstChildren p cs = state $ \h -> let nch = M.alter (\Nothing -> Just cs) p (first_children h)


this state could be a modify :: (s -> s) -> State s ()

croyzor · 2026-01-05T15:33:44Z

brat/Data/HugrGraph.hs

+setOp :: NodeId -> HugrOp -> State HugrGraph ()
+-- Insist the parent exists
+setOp name op = state $ \h@HugrGraph {parents, nodes} -> case M.lookup name parents of
+  Nothing -> error "name has no parent"


Suggested change

Nothing -> error "name has no parent"

Nothing -> error ("Node " ++ show name ++ " has no parent")

croyzor · 2026-01-05T15:35:13Z

brat/Data/HugrGraph.hs

+              parents = M.alter (\Nothing -> Just parent) (NodeId freshName) parents
+            })
+
+setFirstChildren :: NodeId -> [NodeId] -> State HugrGraph ()


Suggested change

setFirstChildren :: NodeId -> [NodeId] -> State HugrGraph ()

-- INVARIANT: first children for this node must not already be set

setFirstChildren :: NodeId -> [NodeId] -> State HugrGraph ()

Invariants are true afterwards too ;-). "ERRORS if already set"....in Rust this would be "PANICS if alreday set"

croyzor · 2026-01-05T15:35:47Z

brat/Data/HugrGraph.hs

+setFirstChildren p cs = state $ \h -> let nch = M.alter (\Nothing -> Just cs) p (first_children h)
+                                      in ((), h {first_children = nch})
+
+setOp :: NodeId -> HugrOp -> State HugrGraph ()


Suggested change

setOp :: NodeId -> HugrOp -> State HugrGraph ()

-- INVARIANT: The node op must not already be set

setOp :: NodeId -> HugrOp -> State HugrGraph ()

croyzor · 2026-01-06T16:30:55Z

brat/Brat/Compile/Hugr.hs

-    defNode <- addNode (show fnName ++ "_def") (OpDefn $ FuncDefn moduleNode (show fnName) funTy [])
-    registerFuncDef idNode (defNode, extra_call)
-    pure (body defNode)
+    ctr@Ctr {parent} <- freshNodeWithIO (show fnName ++ "_def") moduleNode


retain calling the parent "defNode" for clarity pls

croyzor · 2026-01-09T14:49:27Z

brat/Data/HugrGraph.hs

+    nodeStackAndIndices :: StackAndIndices
+    nodeStackAndIndices = let just_root = (B0 :< (root, nodes M.! root), M.singleton root 0)
+                              init = foldl addNode just_root (first_children root)
+                          in foldl addNode init (M.keys parents)


Haskell folklore usually goes "Use foldr instead of foldl. If you really want foldl, use foldl1." Though I looked into it and couldn't convince myself that this use of foldl is problematic

You mean foldl' not foldl1 but done, also combined the two uses - copying first_children root is harmless, it'll be at most two elements!

croyzor · 2026-01-12T10:59:56Z

LGTM

Uses the HugrGraph ADT added in #97. `splice` replaces a HoleOp (ignoring the index) with a DFG-rooted Hugr of matching signature, i.e. inserts the DFG. `inlineDFG` flattens the result, if desired. I wasn't sure what the best approach was for dealing with new/old keys, but the `splice` method is general over both key types by taking a translation function, which gives some guarantee that we *are* translating the keys. `splice_prepend` (both NodeID Hugr's) and `splice_new` (arbitrary-keyed into NodeID) offer two possibilities. My hope ATM is that we don't need to deal with order edges since these are *only* added for nonlocal edges, and so we can do splicing/inlining *before* adding order edges. Tests are pretty basic here, i.e. about the simplest possible case, with/without inline.

acl-cqc added 24 commits December 17, 2025 12:40

Hugr: move parent pointer outside of node

e3e8260

Make abstract datatype for Hugr

8b26471

WIP inline the one -!, abandon FreshMonad

990e261

Fix Hugr on NodeId, move Namespace inside -> freshNodeWithParent

214333b

WIP inline withIO

33a498c

refactor: lift compileNode up to top level

99a7416

Revert "refactor: lift compileNode up to top level"

5ec4c67

This reverts commit 99a7416.

Separate out compilation of Source - fails 3 tests

b9be70d

Fix Eval not respecting liftedOutPorts -> just 1 fail

4f3d1ff

And splices should use getOutPort too

5e4ee49

xfail remaining failure in closures.brat

af49caf

fixup! Separate out

8b74892

refactor out compileInEdges

1e9e1f2

refactor in_edges to top-level

33eb452

Separate out compilation of Target

d42778d

refactor: compileKernBox takes src+tgt (Name, Name)

57c2df7

Rename: freshNodeWithParent -> freshNode

41b913f

makeCS takes Hugr

df8b4dd

compileBox takes Container = 3*NodeId, uses setOp; {freshNode,new}WithIO

1cfebd1

HugrGraph: separate out root

57f2110

HugrGraph: add io_children (written not read)

a791530

WIP use io_children to sort Hugr. Fails on Conditionals - cases in wr…

e6cecd7

…ong order??

hack setFirstChildren to get Cases in order inside Conditional

af23d7e

Drop unused instance Ord, and index inside OpCase

5d0d815

acl-cqc marked this pull request as ready for review December 18, 2025 13:36

acl-cqc force-pushed the refactor/hugr_graph branch from b242b34 to 0651887 Compare December 20, 2025 09:24

acl-cqc requested a review from croyzor December 20, 2025 09:28

acl-cqc added 3 commits December 20, 2025 10:09

Hugr->HugrGraph

d7ae859

Remove HugrGraph.freshNodeWithIO

c2cee45

Move Container from HugrGraph to Compile/Hugr

cf5f705

acl-cqc added 3 commits December 20, 2025 10:09

new returns just HugrGraph, expose root accessor

c50cbb7

io_children->first_children stores any number

3f3c06d

ake HugrGraph ops in State HugrGraph, Compile use onHugr

365dddc

acl-cqc force-pushed the refactor/hugr_graph branch from 0651887 to 365dddc Compare December 20, 2025 10:12

acl-cqc added 5 commits December 22, 2025 17:46

HugrGraph.hs: do not re-export PortId

cb31461

reduce imports from Naming

b88878a

Move renameAndSort inside serialize

446d978

Honour first_children of root

cac91fe

refactor previous

4b25c84

acl-cqc mentioned this pull request Dec 24, 2025

feat: add splice on HugrGraph ADT #98

Merged

croyzor approved these changes Jan 9, 2026

View reviewed changes

acl-cqc added 8 commits January 10, 2026 08:55

Review comment: M.alter in addToMap

5416be8

Review comment: check only single in-edges

36ae9d1

Review comments: state->modify, parent->defNode, foldl'

a4523af

comments

f691012

WIP HugrGraph.hs separate Namespace

d2e5473

Move Namespace out of HugrGraph into Compile monad

10e3abd

Parametrize HugrGraph by nodeid for methods that don't use Namespace

673d664

hide HG.edgeList

1e34f29

acl-cqc requested a review from croyzor January 10, 2026 10:18

acl-cqc mentioned this pull request Jan 12, 2026

Malformed Brat Graph in closures.brat #101

Open

comment issue 101

00b4b16

acl-cqc force-pushed the refactor/hugr_graph branch from 93d9302 to 00b4b16 Compare January 12, 2026 19:59

acl-cqc merged commit 70ff5bf into main Jan 12, 2026
1 check passed

acl-cqc deleted the refactor/hugr_graph branch January 12, 2026 20:06

	Nothing -> error "name has no parent"
	Nothing -> error ("Node " ++ show name ++ " has no parent")

	setFirstChildren :: NodeId -> [NodeId] -> State HugrGraph ()
	-- INVARIANT: first children for this node must not already be set
	setFirstChildren :: NodeId -> [NodeId] -> State HugrGraph ()

	setOp :: NodeId -> HugrOp -> State HugrGraph ()
	-- INVARIANT: The node op must not already be set
	setOp :: NodeId -> HugrOp -> State HugrGraph ()

Conversation

acl-cqc commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

croyzor commented Jan 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

acl-cqc commented Dec 17, 2025 •

edited

Loading

acl-cqc Jan 10, 2026 •

edited

Loading

acl-cqc Jan 10, 2026 •

edited

Loading

acl-cqc Jan 10, 2026 •

edited

Loading