Skip to content
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

feat!: don't assume "main"-function in dataflow + constant folding #1896

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Jan 29, 2025

Instead, move towards explicitly stating entry-points, in common with remove_dead_funcs and remove_dead_code in #1897 - note this PR follows #1897.

I've also tried to reorganise the code a bit better wrt. prepopulate_df_inputs as we can also handle Conditional and CFG nodes by using existing code, hence renaming to just prepopulate_inputs

BREAKING CHANGE: dataflow analysis on Module-rooted Hugrs will no longer accept input PartialValues for the main function, these should be passed to prepopulate_inputs instead. Also prepopulate_df_inputs is gone, again use prepopulate_inputs. ConstantFoldPass::with_inputs now takes the entry-point node as first parameter as well as the inputs to apply there.

@acl-cqc acl-cqc requested a review from a team as a code owner January 29, 2025 12:16
@acl-cqc acl-cqc requested a review from doug-q January 29, 2025 12:16
@hugrbot
Copy link
Collaborator

hugrbot commented Jan 29, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/inherent_method_missing.ron

Failed in:
Machine::prepopulate_df_inputs, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-passes/src/dataflow/datalog.rs:51

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/method_parameter_count_changed.ron

Failed in:
hugr_passes::const_fold::ConstantFoldPass::with_inputs now takes 3 parameters instead of 2, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-passes/src/const_fold.rs:75

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.52%. Comparing base (cb7bf75) to head (9f477b0).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           acl/const_fold_dce    #1896      +/-   ##
======================================================
- Coverage               86.54%   86.52%   -0.02%     
======================================================
  Files                     195      195              
  Lines                   35652    35594      -58     
  Branches                32465    32407      -58     
======================================================
- Hits                    30854    30799      -55     
+ Misses                   3011     3009       -2     
+ Partials                 1787     1786       -1     
Flag Coverage Δ
python 92.34% <ø> (ø)

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.

let mut p = in_values.into_iter().peekable();
// We must provide some inputs to the root so that they are Top rather than Bottom.
// (However, this test will fail for DataflowBlock or Conditional roots, i.e. if no
// inputs have been provided they will still see Bottom. We could store the "input"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very long note to have as a "We could" comment, it'll probably just get out-of-date ;). Perhaps I should just do it now...

@acl-cqc acl-cqc force-pushed the acl/dataflow_no_main branch from 733de9c to 9f477b0 Compare January 29, 2025 17:17
@acl-cqc acl-cqc changed the base branch from main to acl/const_fold_dce January 29, 2025 17:17
@acl-cqc acl-cqc changed the title feat!: remove implicit "main"-function support from dataflow analysis feat!: don't assume "main"-function in dataflow + constant folding Jan 29, 2025
@acl-cqc acl-cqc marked this pull request as draft February 3, 2025 16:51
@acl-cqc acl-cqc changed the base branch from acl/const_fold_dce to main February 3, 2025 16:51
@acl-cqc
Copy link
Contributor Author

acl-cqc commented Feb 3, 2025

Will make this a follow-on to #1901

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.

2 participants