-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Introduce new enum for constant folding; deprecate Value::Function #2060
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
Draft
acl-cqc
wants to merge
28
commits into
release-rs-v0.16.0
Choose a base branch
from
acl/foldval2
base: release-rs-v0.16.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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)
Base automatically changed from
acl/dataflow_call_indirect
to
release-rs-v0.16.0
April 16, 2025 10:11
…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:
|
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::Value
for constant-folding, which is a bit of a shortcut but leads to significant problems: the result ofLoadFunction
cannot 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
FoldVal
that looks quite similar toValue
but adds those references/function-pointers.I've taken the liberty of not including nested Hugrs in
FoldVal
, but rather deprecatingValue::Function
in 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_fold
andfold
routines 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 fold
does 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