feat!: Handle CallIndirect in Dataflow Analysis by acl-cqc · Pull Request #2059 · Quantinuum/hugr

acl-cqc · 2025-04-04T15:01:28Z

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 :-(. The alternative is to restrict all dataflow analysis to work only on HugrViews that use Node.
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

…dCtx ignores self.0)

codecov · 2025-04-04T15:04:21Z

Codecov Report

Attention: Patch coverage is 94.60581% with 13 lines in your changes missing coverage. Please review.

Project coverage is 83.01%. Comparing base (fbbf237) to head (887a386).
Report is 13 commits behind head on release-rs-v0.16.0.

Files with missing lines	Patch %	Lines
hugr-passes/src/dataflow/partial_value.rs	96.10%	2 Missing and 1 partial ⚠️
hugr-passes/src/dataflow/test.rs	88.00%	3 Missing ⚠️
hugr-passes/src/const_fold.rs	80.00%	2 Missing ⚠️
hugr-passes/src/const_fold/value_handle.rs	75.00%	2 Missing ⚠️
hugr-passes/src/dataflow.rs	60.00%	2 Missing ⚠️
hugr-passes/src/dataflow/datalog.rs	99.06%	1 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           release-rs-v0.16.0    #2059      +/-   ##
======================================================
+ Coverage               82.91%   83.01%   +0.09%     
======================================================
  Files                     217      217              
  Lines                   41529    41654     +125     
  Branches                37707    37832     +125     
======================================================
+ Hits                    34433    34577     +144     
+ Misses                   5292     5273      -19     
  Partials                 1804     1804

Flag	Coverage Δ
python	`85.40% <ø> (ø)`
rust	`82.76% <94.60%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hugrbot · 2025-04-04T15:06:39Z

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary


--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
      ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/auto_trait_impl_removed.ron

Failed in:
type PartialSum is no longer UnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:67
type PartialSum is no longer RefUnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:67
type AnalysisResults is no longer UnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/results.rs:11
type AnalysisResults is no longer RefUnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/results.rs:11
type PartialValue is no longer UnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:314
type PartialValue is no longer RefUnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:314
type Machine is no longer UnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/datalog.rs:31
type Machine is no longer RefUnwindSafe, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/datalog.rs:31

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/enum_variant_added.ron

Failed in:
variant PartialValue:LoadedFunction in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:318

--- failure function_requires_different_generic_type_params: function now requires a different number of generic type parameters ---

Description:
A function now requires a different number of generic type parameters than it used to. Uses of this function that supplied the previous number of generic types (e.g. via turbofish syntax) will be broken.
      ref: https://doc.rust-lang.org/reference/items/generics.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/function_requires_different_generic_type_params.ron

Failed in:
function row_contains_bottom (1 -> 2 generic types) in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow.rs:124

--- failure method_requires_different_generic_type_params: method now requires a different number of generic type parameters ---

Description:
A method now requires a different number of generic type parameters than it used to. Uses of this method that supplied the previous number of generic types will be broken.
      ref: https://doc.rust-lang.org/reference/items/generics.html
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.40.0/src/lints/method_requires_different_generic_type_params.ron

Failed in:
hugr_passes::dataflow::PartialSum::try_into_sum takes 1 generic types instead of 3, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:202
hugr_passes::dataflow::AnalysisResults::try_read_wire_concrete takes 1 generic types instead of 3, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/results.rs:90
hugr_passes::dataflow::PartialValue::try_into_concrete takes 1 generic types instead of 3, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/dataflow/partial_value.rs:417

hugr-passes/src/dataflow/results.rs

hugr-passes/src/const_fold.rs

acl-cqc · 2025-04-04T17:17:39Z

hugr-passes/src/dataflow/datalog.rs

        OpType::Input(_) | OpType::Output(_) | OpType::ExitBlock(_) => None, // handled by parent
-        OpType::Call(_) => None,  // handled via Input/Output of FuncDefn
-        OpType::Const(_) => None, // handled by LoadConstant:
+        OpType::Call(_) | OpType::CallIndirect(_) => None, // handled via Input/Output of FuncDefn


I dropped Const: it's not a dataflow node (it's a module/static), so never gets here

acl-cqc · 2025-04-04T17:19:58Z

hugr-passes/src/dataflow/partial_value.rs

+            (Self::LoadedFunction(lf1), Self::LoadedFunction(lf2))
+                if lf1.func_node == lf2.func_node =>
+            {
+                // TODO we should also require TypeArgs to be equal by at the moment these are ignored


I.e. we are ignoring the TypeArgs in Call nodes too. I'll make an issue, but not this PR.

acl-cqc · 2025-04-04T17:22:14Z

hugr-passes/src/dataflow/partial_value.rs

 /// of values of the underlying representation
 #[derive(PartialEq, Clone, Eq, Hash, Debug)]
-pub enum PartialValue<V> {
+pub enum PartialValue<V, N = Node> {


Not sure about the default N=Node. To get everything to work I ended up searching for a regex of PartialValue with only one thing between the <>'s...hence putting in a few explicit , Nodes into tests. I could retract those and use the default, or remove the default....

(for now I've removed the explicit , Node in tests, but easy to reinstate, and still considering whether to remove the default)

…irect

… Debug

doug-q

Looks great Alan. I think there's a mistake in the indirect_call datalog

doug-q · 2025-04-07T07:16:05Z

hugr-passes/src/dataflow/datalog.rs

            in_wire_value(outp, p, v);
+
+        // CallIndirect --------------------
+        relation indirect_call(H::Node, H::Node); // <Node> is an `IndirectCall` to `FuncDefn` <Node>


I think this needs to be a lattice, and to store the Callee.

As written, if you first get a LoadedFunction, then it goes to TOP, you'll not update the out_wire_values to top.

I think you should deal with polymorphism either by requring 0 type args

Great point about it needing to be a lattice, thank you! :). Done.
(Annoying about not having a test to show the difference but I bet it'd have show up sooner or later.)

Polymorphism is not wrong atm. Values from different type-instantiations are just joined together so will likely produce Top. If there's a LoadNat - well we don't even handle that yet - but any DFContext::interpret_leaf_op will just see a LoadNat<Var0> and not get the type args, so there's no sensible implementation other than returning Top. It's not ideal, but it's not wrong.

doug-q · 2025-04-07T07:17:27Z

hugr-passes/src/dataflow/datalog.rs

        OpType::Input(_) | OpType::Output(_) | OpType::ExitBlock(_) => None, // handled by parent
-        OpType::Call(_) => None,  // handled via Input/Output of FuncDefn
-        OpType::Const(_) => None, // handled by LoadConstant:
+        OpType::Call(_) | OpType::CallIndirect(_) => None, // handled via Input/Output of FuncDefn


I think it would be more consistent to deal with callindirect here rather than in the datalog, is that possible?

Only by a massive refactor of propagate_leaf_op. Which is private, so we could, but I don't feel this is inconsistent - I think it's just like what we have been doing for Call

Not worth a massive refactor. I think it's inconsistent only because I don't think "CallIndirect" is morally built in(I claim it could be in prelude)

doug-q · 2025-04-07T07:19:39Z

hugr-passes/src/dataflow/partial_value.rs

+    }
+
+    /// Can this ever occur at runtime? See [PartialValue::contains_bottom]
+    pub fn contains_bottom(&self) -> bool {


This is confusing. I would expect any here, but I think this really means "surely has at least one bottom field"

Yes - "definitely contains bottom" not "might contain bottom". I think the best thing to do is rename, which I could do here (this PR is breaking already, of course I could deprecate)...or perhaps a doc update would suffice?

Happy to deprecate and "rename" to anything we can agree on. definitely_contains_bottom seems too long (esp. with row_ prefix). I don't really like is_bottom, would prefer to invert sense and go with can_occur (which is already noted in comments!) - but IIUC you don't like that. Other opinions welcome, but a rename with deprecation is not breaking so could follow afterwards.

doug-q · 2025-04-07T07:23:00Z

hugr-passes/src/dataflow/datalog.rs

-                ctx.value_from_function(func_node, &load_op.type_args)
-                    .map_or(PV::Top, PV::Value),
-            ))
+            Some(ValueRow::singleton(PartialValue::LoadedFunction(


Recommend you use PartialValue::new_load to get coverage on that function

doug-q · 2025-04-07T07:24:36Z

hugr-passes/src/dataflow/partial_value.rs

+    /// A value contains bottom means that it cannot occur during execution:
+    /// it may be an artefact during bootstrapping of the analysis, or else
+    /// the value depends upon a `panic` or a loop that
+    /// [never terminates](super::TailLoopTermination::NeverBreaks).


or occurs in a Case which is never entered, or a function that is never called.

Not yet - e.g. the output of a LoadConst inside an unreachable Case does not contain bottom until #1672 (if ever)

hugr-passes/src/dataflow/results.rs

doug-q

Nice. LatticeWrapper doesn't have test coverage, this would be easy to improve.

hugr-passes/src/dataflow/datalog.rs

* 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.

acl-cqc added 5 commits April 4, 2025 13:20

No need for ConstFoldContext to impl Deref

692a759

Add PartialValue::LoadedFunction

48143f5

Docs, clippy::type_complexity

5809594

datalog handles LoadFunction, drop value_from_function (TODO ConstFol…

cccefe4

…dCtx ignores self.0)

And handle CallIndirect

ca442c4

acl-cqc mentioned this pull request Apr 4, 2025

feat!: Handle CallIndirect in constant folding #2046

Closed

2 tasks

acl-cqc commented Apr 4, 2025

View reviewed changes

hugr-passes/src/dataflow/results.rs Show resolved Hide resolved

acl-cqc commented Apr 4, 2025

View reviewed changes

hugr-passes/src/const_fold.rs Outdated Show resolved Hide resolved

acl-cqc mentioned this pull request Apr 4, 2025

feat: Introduce new enum for constant folding; deprecate Value::Function #2060

Closed

acl-cqc requested a review from doug-q April 4, 2025 17:15

acl-cqc commented Apr 4, 2025

View reviewed changes

acl-cqc added 6 commits April 4, 2025 20:48

First bit of test - default to Top if called-func unknown

a90dc97

Test2, fix port numbering and narrow unknown-called-func case

6e2ad7f

Test3, plus refactor test

2cd8547

refactor into cases of rstest

ba33062

Merge remote-tracking branch 'origin/main' into acl/dataflow_call_ind…

5c9c6ec

…irect

PartialSum default N=Node

ee128ce

acl-cqc marked this pull request as ready for review April 4, 2025 21:33

acl-cqc requested a review from a team as a code owner April 4, 2025 21:33

require TryFrom<LoadedFunction<N>,Error=LoadedFunction<N>>, format w/…

2757a33

… Debug

doug-q reviewed Apr 7, 2025

View reviewed changes

acl-cqc added 3 commits April 7, 2025 10:19

Use PartialValue::new_load

3c79739

Make call_indirect a lattice, add load_func

1191150

Drop Hugr from inside ConstFoldContext

4ba5603

acl-cqc requested a review from doug-q April 8, 2025 12:33

acl-cqc added the S-breaking-change Status: Need a breaking change release to progress label Apr 9, 2025

doug-q approved these changes Apr 14, 2025

View reviewed changes

hugr-passes/src/dataflow/datalog.rs Outdated Show resolved Hide resolved

aborgna-q added the wait to merge label Apr 14, 2025

aborgna-q added this to the hugr-rs 0.16 milestone Apr 14, 2025

acl-cqc added 5 commits April 15, 2025 16:37

trait AsConcrete

a8e4553

LatticeWrapper requires only PartialEq+PartialOrd

f032848

PartialValue: Arbitrary+TestSumType etc. generates LoadedFunction

3066b65

clippy, fix predecence

3eb0c7f

test LatticeWrapper

887a386

acl-cqc mentioned this pull request Apr 15, 2025

Expose result of LoadFunction into constant folding #2087

Open

acl-cqc changed the base branch from main to release-rs-v0.16.0 April 16, 2025 09:58

acl-cqc merged commit baaca02 into release-rs-v0.16.0 Apr 16, 2025
27 checks passed

acl-cqc deleted the acl/dataflow_call_indirect branch April 16, 2025 10:11

This was referenced May 7, 2025

chore: release v0.20.0 #2162

Closed

chore: release v0.20.0 #2181

Closed

hugrbot mentioned this pull request May 14, 2025

chore: release v0.21.0 #2220

Closed

acl-cqc mentioned this pull request May 21, 2025

Devirtualization of CallIndirect -> Call #1052

Open

This was referenced May 29, 2025

chore: release v0.21.0 #2271

Closed

chore: release v0.20.1 #2273

Merged

Conversation

acl-cqc commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hugrbot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-q left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

doug-q left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

acl-cqc commented Apr 4, 2025 •

edited

Loading

codecov bot commented Apr 4, 2025 •

edited

Loading

hugrbot commented Apr 4, 2025 •

edited

Loading