feat: add HierarchyTester for O(1) subtree-containment; rewrite nonlocal edge validation #2232
Conversation
| Self { hugr, dominators } | ||
| Self { | ||
| hugr, | ||
| hierarchy_tester: None, |
There was a problem hiding this comment.
We could also just compute this now, rather than if-needed. Whereas dominators are only needed for Dom edges, the hierarchy tester is needed for both Dom and Ext edges, so more likely. (One might argue that any Hugr that doesn't need it, will probably be so small that computing it is insignificant??)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2232 +/- ##
==========================================
+ Coverage 82.00% 82.03% +0.03%
==========================================
Files 234 235 +1
Lines 41618 41689 +71
Branches 37532 37602 +70
==========================================
+ Hits 34127 34200 +73
Misses 5518 5518
+ Partials 1973 1971 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
acl-cqc
changed the title
| #[allow(unused)] // Just make sure the Hugr isn't changing behind our back | ||
| hugr: &'a H, |
There was a problem hiding this comment.
What if we want to mutate the hugr though? Not all changes affect the hierarchy.
(See e.g. petgraph's Topo iterator)
|
|
||
| /// The entry and exit indices (inclusive, note every entry number is different); | ||
| /// and the nearest strictly-enclosing FuncDefn. | ||
| type NodeData<N> = (usize, usize, Option<N>); |
There was a problem hiding this comment.
This should be a struct with named fields.
There was a problem hiding this comment.
An alternative implementation (maybe a 2nd definition) is lazily computing these values.
See portgraph's Region::is_descendant, which has an amortized O(1) complexity too, and here would let us add nodes after the fact.
I thought it was worth reifying HierarchyTester as there are more uses upcoming and it seems a good thing to have. I'm not sure if there are other places already existing where we could use it...?
However I admit the validation rewrite is not as compelling as I had hoped, due to the requirement for
containing_childcontaining_childfor Ext edges, I expect to eventually disappear, as I think we are planning to remove the requirement for these Order edges (some other utility will compute the necessary topsort order that puts sources before nodes containing their targets)containing_childfor Dom edges could be removed by computing the same start/stop indices as used in HierarchyTester, but taking dominance into account i.e. by traversing the dominator tree. The downside is then we'd most likely have to compute dominators for every CFG in the Hugr as soon as we need any HierarchyTester-ing, rather than (what we do now:) for each CFG as/if we need it. I could do this in this PR, but haven't ATM.