feat: Introduce new enum for constant folding; deprecate Value::Function#2060
feat: Introduce new enum for constant folding; deprecate Value::Function#2060acl-cqc wants to merge 18 commits intoacl/insert_hugr_defnsfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## acl/insert_hugr_defns #2060 +/- ##
=========================================================
+ Coverage 82.18% 82.36% +0.18%
=========================================================
Files 239 240 +1
Lines 43081 43770 +689
Branches 38991 39680 +689
=========================================================
+ Hits 35405 36052 +647
- Misses 5708 5753 +45
+ Partials 1968 1965 -3
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
acl-cqc
force-pushed
the
acl/foldval2
branch
2 times, most recently
from
eaaa3c6 to
d0571af
Compare
CodSpeed Performance ReportMerging #2060 will not alter performanceComparing Summary
|
ss2165
left a comment
ss2165
left a comment
There was a problem hiding this comment.
Initial thoughts.
There is a lot of deprecation overhead here, not sure how much is worthwhile. If the fold2 names are kept they may need to become deprecated aliases in the breaking release
| /// Representation of values used for constant folding. | ||
| /// See [ConstFold], which is used as `dyn` so we cannot parametrize by | ||
| /// [HugrNode](crate::core::HugrNode). | ||
| // Should we be non-exhaustive?? |
There was a problem hiding this comment.
Don't think so, "Extension" should be covering the points of extension
| tag: usize, | ||
| /// Describes the type of the whole value. | ||
| // Can we deprecate this immediately? It is only for converting to Value | ||
| sum_type: SumType, |
There was a problem hiding this comment.
Could move these fields to an inner struct and mark them private i.e. Sum(SumVal)
| /// A constant value defined by an extension | ||
| Extension(OpaqueValue), | ||
| /// A function pointer loaded from a [FuncDefn](crate::ops::FuncDefn) or `FuncDecl` | ||
| LoadedFunction(Node, Vec<TypeArg>), // Deliberately skipping Function(Box<Hugr>) ATM |
There was a problem hiding this comment.
do we need non_exhaustive for Function(Box<Hugr>), or a stub implementation that errors?
| } | ||
| } | ||
|
|
||
| /// Extract the specified type of [CustomConst] fro this instance, if it is one |
There was a problem hiding this comment.
| /// Extract the specified type of [CustomConst] fro this instance, if it is one | |
| /// Extract the specified type of [CustomConst] for this instance, if it is one |
| } | ||
|
|
||
| impl TryFrom<FoldVal> for Value { | ||
| type Error = Option<Node>; |
There was a problem hiding this comment.
please add docstring to explain error type
| /// | ||
| /// Defaults to calling [Self::fold] with those arguments that can be converted --- | ||
| /// [FoldVal::LoadedFunction]s will be lost as these are not representable as [Value]s. | ||
| fn fold2(&self, type_args: &[TypeArg], inputs: &[FoldVal], outputs: &mut [FoldVal]) { |
There was a problem hiding this comment.
why does this follow a mutable output pattern? Would some other process have set some values already that this one would leave unchanged?
| .map(TestVal::to_value) | ||
| .map(FoldVal::from) |
There was a problem hiding this comment.
seems to make more sense to update to_value to go directly to FoldVal?
|
The problem that FoldVal != Value renders CustomConsts for containers useless....is fatal, I think. However deprecation of Value::Function has at least happened in #2770 so closing. |
2 participants
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
ConstantFoldertrait; deprecating a method in a trait only triggers a warning when you call it, not when you implement it, and we'd want people to move to implementing the new method.However, currently stuck on:
Sum<T>from dataflow, and to remove the SumType from that, but that migration doesn't look very graceful :-(ListValuecause trouble as they containValue- so, no lists of function pointers. This'll be somewhat messy too.hugr-model's improved constants...closes: #2087, #1856