Skip to content

feat!: Handle CallIndirect in constant folding #2046

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

Closed
wants to merge 11 commits into from

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Mar 31, 2025

  • Extend ValueHandle with a node pointer into the containing Hugr - the previous approach of handling LoadFunction by stealing the body of the loaded function, will not work for functions with incoming static edges (i.e. from other functions/constants).
  • Add new interpret_call_indirect method to trait DFContext (alongside interpret_leaf_op), again defaulting to { }
  • Implement for ConstFoldContext by a complete recursion (i.e. when the called function is known).
  • TODO: tests!
  • This recursion may be expensive (and seems more than we do for calls - multiple CallIndirects to the same function will each get their own analysis, without aliasing, whereas multiple Calls to the same function get corresponding arguments/intermediates joined together). Should be put it under a switch?

BREAKING CHANGE: mod value_handle (containing ValueHandle and HashedConst) is no longer pub.

Copy link

codecov bot commented Mar 31, 2025

Codecov Report

Attention: Patch coverage is 3.33333% with 58 lines in your changes missing coverage. Please review.

Project coverage is 83.21%. Comparing base (6fff648) to head (482a435).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
hugr-passes/src/const_fold.rs 0.00% 39 Missing ⚠️
hugr-passes/src/dataflow.rs 0.00% 7 Missing ⚠️
hugr-passes/src/const_fold/value_handle.rs 25.00% 6 Missing ⚠️
hugr-passes/src/dataflow/datalog.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2046      +/-   ##
==========================================
+ Coverage   83.17%   83.21%   +0.04%     
==========================================
  Files         215      217       +2     
  Lines       41312    41910     +598     
  Branches    37526    38124     +598     
==========================================
+ Hits        34362    34877     +515     
- Misses       5006     5079      +73     
- Partials     1944     1954      +10     
Flag Coverage Δ
python 85.92% <ø> (ø)
rust 82.95% <3.33%> (+0.05%) ⬆️

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
Copy link
Collaborator

hugrbot commented Mar 31, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- 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 ValueHandle:NodeRef in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/const_fold/value_handle.rs:58

@acl-cqc acl-cqc requested a review from aborgna-q April 1, 2025 09:17
Copy link
Collaborator

@aborgna-q aborgna-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just missing the tests.

This recursion may be expensive (...). Should be put it under a switch?

You mean to enabling/disabling callindirect folding altogether?
I'd say we can wait and see if it's necessary, but I guess we could add a ConstFoldConfig struct here since we're breaking the API already.

@@ -2,18 +2,15 @@
//! Constant-folding pass.
//! An (example) use of the [dataflow analysis framework](super::dataflow).

pub mod value_handle;
mod value_handle;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite annoying that we have to make this a breaking change.
It looks like even leaving this as public wouldn't help, since ValueHandle is not non_exhaustive and added new fields -.-

Is it OK to wait in merging this PR until we accumulate other breaking changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was hiding that because I thought that's the way that it should have been, would have avoided this being a breaking change now...

How urgent....I was gonna say, the bit we need first is the constant_fold_with_hugr and that's non-breaking so I can break that out and leave the rest as a demo (just tests that the ValueHandle/CallIndirect works would confirm we can do the same approach in/for BRAT).

However....now I realize we need to get the ValueHandle::NodeRef into the BRAT-specific constant-folding code. At present extension-provided constant-folders only get Hugr Values, so rather than adding constant_fold_with_hugr I think I need some kind of plugin interface to the constant-folding pass that lets us get at those ValueHandles after all. So I think I'll have to restore pubness and figure out what the plugin interface is.

@acl-cqc
Copy link
Contributor Author

acl-cqc commented Apr 4, 2025

Closing in favour of #2059

@acl-cqc acl-cqc closed this Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants