feat: InlineFunctionsPass supports "Always" annotation that errors if unsatisfiable#1449
feat: InlineFunctionsPass supports "Always" annotation that errors if unsatisfiable#1449acl-cqc wants to merge 44 commits into
Conversation
aborgna-q
force-pushed
the
ab/hugr-0.26.0
branch
10 times, most recently
from
cb4d32c to
2b29cdb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
+ Coverage 85.29% 85.85% +0.56%
==========================================
Files 191 194 +3
Lines 30355 30927 +572
Branches 28986 29518 +532
==========================================
+ Hits 25890 26552 +662
+ Misses 3240 3108 -132
- Partials 1225 1267 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aborgna-q
left a comment
aborgna-q
left a comment
There was a problem hiding this comment.
Some comments, mostly about differentiating the two passes.
Although from the technical side it makes sense to have two of them (we need to do different kinds of traversals for each), I struggle to justify having the split passes from the user point of view.
When would I want to "inline some of the annotated functions, but not the others that I've also annotated"?
acl-cqc
changed the title
Yeah, totally agree. Have merged the two. I didn't really go looking for common code and I imagine there must be at least some but see how you think this looks now? |
| Ok(()) | ||
| } | ||
|
|
||
| fn inline_always_scoped<H: HugrMut>( |
| Ok(()) | ||
| } | ||
|
|
||
| fn cycles<'a, N: Copy>( |
| /// Always inline calls to this function. | ||
| /// | ||
| /// If this cannot be done, an error will be raised. | ||
| Always, |
There was a problem hiding this comment.
The semver checks are failing because this was added before all existing variants, and changed their inner tag id.
It also breaks the Ord derive.
Can you move the new variant to the end?
Alternatively, do
Always = 2,
BestEffort = 0,| for child in hugr.children(parent) { | ||
| if hugr.first_child(child).is_some() { | ||
| parents.push_back(child); | ||
| } else if hugr.get_optype(child).is_call() |
There was a problem hiding this comment.
We're not handling LoadFunctions here, even though the reachable_always logic above does add them
tket2/tket/src/passes/inline_funcs.rs
Lines 202 to 203 in 566dc2b
There was a problem hiding this comment.
That's a really good point, thank you! Should ignore LoadFunction edges when looking for cycles, as they can't be inlined (yet we push them onto to_inline, ooops); and not delete functions with LoadFunctions remaining. Quite a few bugs there, which tests including LoadFunction would have spotted.... I've updated code but not added the tests. I think we'll have to let this one slip to a later release....it is non-breaking after all.
2 participants
Intended as precursor to #1532: this adds the "Always" option only, but takes into account in its own pass, which raises an error if it cannot obey all the annotations.
Python interface is very WIP, not sure I've grokked everything there yet.