feat: Introduce new enum for constant folding; deprecate Value::Function #2060
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…dCtx ignores self.0)
`insert_hugr`, `insert_from_view`, and `insert_subgraph` were written before we made `Node` a type generic, and incorrectly assumed the return type maps were always `hugr::Node`s. The methods were either unusable or incorrect when using generic `HugrView`s source/targets with non-base node types. This PR fixes that, and additionally allows us us to have `SiblingSubgraph::extract_subgraph` work for generic `HugrViews`. BREAKING CHANGE: Added Node type parameters to extraction operations in `HugrMut`.
`HugrMutInternals` is part of the semi-private traits defined in `hugr-core`. While most things get re-exported in `hugr`, we `*Internal` traits require you to explicitly declare a dependency on the `-core` package (as we don't want most users to have to interact with them). For some reason there was a public re-export of the trait in a re-exported module, so it ended up appearing in `hugr` anyways. BREAKING CHANGE: Removed public re-export of `HugrMutInternals` from `hugr`.
#2027 ended up being breaking due to adding a new variant to an error enum missing the `non_exhaustive` marker. This (breaking) PR makes sure all error enums have the flag. BREAKING CHANGE: Marked all Error enums as `non_exhaustive`
* PartialValue now has a LoadedFunction variant, created by LoadFunction nodes (only, although other analyses are able to create PartialValues if they want) * This requires adding a type parameter to PartialValue for the type of Node, which gets everywhere :-(. * Use this to handle CallIndirects *with known targets* (it'll be a single known target or none at all) just like other Calls to the same function * deprecate (and ignore) `value_from_function` * Add a new trait `AsConcrete` for the result type of `PartialValue::try_into_concrete` and `PartialSum::try_into_sum` Note almost no change to constant folding (only to drop impl of `value_from_function`) BREAKING CHANGE: in dataflow framework, PartialValue now has additional variant; `try_into_concrete` requires the target type to implement AsConcrete.
Base automatically changed from
acl/dataflow_call_indirect
to
release-rs-v0.16.0
April 16, 2025 10:11
Adds a generic node type to the `NodeHandle` type. This is a required change for #2029. drive-by: Implement the "Link the NodeHandles to the OpType" TODO
Closes #1595 BREAKING CHANGE: `values` field in `Extension` and `ExtensionValue` struct/class removed in rust and python. Use 0-input ops that return constant values.
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 means `with_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 but `add_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() requires `use ...ComposablePass` (however, such calls will cease to do any validation). closes #1832
…eady in the Hugr (#2094) There are two issues: * Errors. The previous NodeTemplates still always work, but the Call one can fail if the Hugr doesn't contain the target function node. ATM there is no channel for reporting that error so I've had to panic. Otherwise it's an even-more-breaking change to add an error type to `NodeTemplate::add()` and `NodeTemplate::add_hugr()`. Should we? (I note `HugrMut::connect` panics if the node isn't there, but could make the `NodeTemplate::add` builder method return a BuildError...and propagate that everywhere of course) * There's a big limitation in `linearize_array` that it'll break if the *element* says it should be copied/discarded via a NodeTemplate::Call, as `linearize_array` puts the elementwise copy/discard function into a *nested Hugr* (`Value::Function`) that won't contain the function. This could be fixed via lifting those to toplevel FuncDefns with name-mangling, but I'd rather leave that for #2086 .... BREAKING CHANGE: Add new variant NodeTemplate::Call; LinearizeError no longer derives Eq.
…flow_call_indirect
…aflow_call_indirect
…sConcrete) Note we've deprecated Value::Function used heavily in linearize_array.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-rs-v0.16.0 #2060 +/- ##
======================================================
- Coverage 83.33% 83.28% -0.06%
======================================================
Files 219 218 -1
Lines 42205 42223 +18
Branches 38307 38428 +121
======================================================
- Hits 35173 35164 -9
- Misses 5221 5248 +27
Partials 1811 1811
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:
|
aborgna-q
force-pushed
the
release-rs-v0.16.0
branch
from
56c110c to
9cd24ff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've been using
ops::Valuefor constant-folding, which is a bit of a shortcut but leads to significant problems: the result ofLoadFunctioncannot fit into a Value (you'd need to copy all other external functions used by the loaded one inside it, or something - not really practical, and you've lost the understanding that you were calling another function in the same Hugr, too). #2059 solved this inside dataflow analysis, but this extends the approach to constant folding, by allowing to feeding "function pointers" (from LoadFunction) into constant-folding.The "solution" of extending Value to allow a reference to a node in the containing Hugr was considered in #1856 and was roundly rejected. Instead, this PR adds a separate enum
FoldValthat looks quite similar toValuebut adds those references/function-pointers.I've taken the liberty of not including nested Hugrs in
FoldVal, but rather deprecatingValue::Functionin favour of getting front-ends to lift these into their own FuncDefn's in the Hugr. I could be persuaded not to, and add a nested-Hugr variant to FoldVal, if we really want, but it does seem like probably more effort than it's worth - does anyone really have a good use case forValue::Function?I've also deprecated the old
constant_foldandfoldroutines in favour of the new ones, which are currently calledfold2. Better naming suggestions are welcome....(an alternative might be to define a new version of the ConstantFolder trait with only the new method, make OpDef store one of those, deprecate the old trait, and thenimpl<T:ConstantFolder> NewConstantFolder for T?)NOTE/TODO: implementing the deprecated
fn folddoes not trigger a deprecation warning (!), only calling itAlso note: this is not intended to be part of the v0.16.0 release; it can be a non-breaking followup, but I'm targetting the release branch just because this depends on things in that release.
closes: #2087, #1856