feat: add inlining annotation + pass#1449
Conversation
cb4d32c to
2b29cdb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files
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:
|
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
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"?
| f_id.set_outputs(f_id.input_node[0]) | ||
|
|
||
| if annotate: | ||
| f_id.metadata["tket.inline"] = "always" |
There was a problem hiding this comment.
| f_id.metadata["tket.inline"] = "always" | |
| f_id.metadata[InlineAnnotation] = "always" |
| InlineAnnotationValue: TypeAlias = Literal["never", "best_effort", "always"] | ||
|
|
||
|
|
||
| class InlineAnnotation(Metadata[InlineAnnotationValue]): |
There was a problem hiding this comment.
This needs updates in the docs and in from_json
| /// A [ComposablePass] that inlines `Call`s to functions | ||
| /// according to [InlineAnnotation]s. | ||
| #[derive(Default, Clone, Debug)] | ||
| pub struct InlineAlwaysPass { |
There was a problem hiding this comment.
We should explain what are the differences between this pass an InlineFunctionsPass, not only here but in the other pass' docs and on the python definitions too.
| Some(InlineAnnotation::Never) => false, | ||
| Some(InlineAnnotation::BestEffort) => true, | ||
| None => self.heuristic.should_inline(func, h), | ||
| Some(InlineAnnotation::Always) // instruction to InlineAlwaysPass |
There was a problem hiding this comment.
I'm thinking we should treat Always as BestEffort here. We won't inline every occurrence, but at least we should inline the ones we can.
| pub enum InlineAnnotation { | ||
| /// Always inline calls to this function. | ||
| /// | ||
| /// If this cannot be done, an error will be raised. |
There was a problem hiding this comment.
Would probably merit a mention to the different inline passes.
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.