-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: ComposablePass trait allowing sequencing and validation #1895
base: main
Are you sure you want to change the base?
Conversation
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1895 +/- ##
==========================================
+ Coverage 83.74% 83.76% +0.02%
==========================================
Files 204 208 +4
Lines 38585 39015 +430
Branches 35395 35723 +328
==========================================
+ Hits 32312 32680 +368
- Misses 4472 4498 +26
- Partials 1801 1837 +36
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:
|
hugr-passes/src/composable.rs
Outdated
|
||
fn sequence( | ||
self, | ||
other: impl ComposablePass<Err = Self::Err>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way to do this is
fn sequence_before<P2: ComposablePass>(self, other: P2) -> impl ComposablePass<Err = Self::Err>
where P2::Err : Into<Self::Err>
(and similarly sequence_after
) which avoids what may be a common-case use of map_err
(i.e. with Into
).
However, Infallible doesn't implement Into, so it's probably not much of a win. (I could add sequence_infallible(self, other: ComposablePass<Err=Infallible>)
I guess, but would then want that before/after versions of that too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I do think some more docs would be good. Regarding the suggestion to add add_entry_point
-- does it mean to apply the pass to everything below that point in the hierarchy but not above it? I think that sounds reasonable.
Breaking, so will leave merging until after end March |
Currently We have several "passes": monomorphization, dead function removal, constant folding. Each has its own code to allow setting a validation level (before and after that pass).
This PR adds the ability chain (sequence) passes;, and to add validation before+after any pass or sequence; and commons up validation code. The top-level
constant_fold_pass
(etc.) functions are left as wrappers that do a single pass with validation only in test.I've left ConstFoldPass as always including DCE, but an alternative could be to return a sequence of the two - ATM that means a tuple
(ConstFoldPass, DeadCodeElimPass)
.I also wondered about including a method
add_entry_point
in ComposablePass (e.g. for ConstFoldPass, that meanswith_inputs
but no inputs, i.e. all Top). I feel this is not applicable to all passes, but near enough. This could be done in a later PR butadd_entry_point
would need a no-op default for that to be a non-breaking change. So if we wouldn't be happy with the no-op default then I could just add it here...Finally...docs are extremely minimal ATM (this is hugr-passes), I am hoping that most of this is reasonably obvious (it doesn't really do a lot!), but please flag anything you think is particularly in need of a doc comment!
BREAKING CHANGE: quite a lot of calls to current pass routines will break, specific cases include (a)
with_validation_level
should be done by wrapping a ValidatingPass around the receiver; (b) XXXPass::run() requiresuse ...ComposablePass
(however, such calls will cease to do any validation).closes #1832