feat: add verification to constant folding #1030

doug-q · 2024-05-13T09:40:54Z

Fixes #996. Fixes test_fold_inarrow tests which were shown to be wrong by verification, and test_fold_int_ops test which was causing constant folding to panic.

doug-q · 2024-05-13T09:53:23Z

I recommend reviewing commit-by-commit. @cqc-alec I'm mostly asking for your view of the test fixes.

codecov · 2024-05-13T09:56:45Z

Codecov Report

Attention: Patch coverage is 90.83969% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 86.31%. Comparing base (c7fb3a7) to head (20b35d6).
Report is 3 commits behind head on main.

Files	Patch %	Lines
hugr/src/algorithm/const_fold.rs	93.69%	5 Missing and 2 partials ⚠️
hugr/src/hugr/views.rs	50.00%	3 Missing ⚠️
...rc/std_extensions/arithmetic/int_ops/const_fold.rs	84.61%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   86.26%   86.31%   +0.05%     
==========================================
  Files          82       82              
  Lines       17264    17234      -30     
  Branches    17264    17234      -30     
==========================================
- Hits        14893    14876      -17     
+ Misses       1565     1538      -27     
- Partials      806      820      +14

Flag	Coverage Δ
rust	`86.31% <90.83%> (+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.

cqc-alec · 2024-05-13T09:58:56Z

hugr/src/std_extensions/arithmetic/int_ops/const_fold/test.rs

    // pseudocode:
    //
    // x0 := int_s<5>(-3);
    // x1 := inarrow_s<5, 4>(x0);
    // output x1 == int_s<4>(-3);


This comment should be removed or rewritten in more general terms.

cqc-alec · 2024-05-13T10:06:52Z

hugr/src/algorithm/const_fold.rs

+}
+
+#[derive(Debug, Clone, Copy, Default)]
+/// TODO


What is TODO?

docs, I missed this one.

cqc-alec · 2024-05-13T10:13:02Z

hugr/src/algorithm/const_fold.rs

+            for rem in removes {
+                if let Ok(const_node) = h.apply_rewrite(rem) {
+                    // if the LoadConst was removed, try removing the Const too.
+                    let _ = h.apply_rewrite(RemoveConst(const_node));


Is it safe to ignore all errors here?

Yes it is, but I'll add a comment to explain why.

cqc-alec · 2024-05-13T10:15:21Z

hugr/src/algorithm/const_fold.rs

@@ -395,4 +462,44 @@ mod test {
        let expected = Value::false_val();
        assert_fully_folded(&h, &expected);
    }
+


Is it worth also including the test from #996 ?

Yes, will do.

doug-q · 2024-05-13T12:44:01Z

Thanks for the review Alec. I will wait until @ss2165 has had a look before I merge, significant changes to his baby.

ss2165

PR looks good, thanks for the fix, main query about whether broader "pass level" design changes should be made in this PR

ss2165 · 2024-05-13T13:39:56Z

hugr/src/algorithm/const_fold.rs

@@ -69,7 +156,10 @@ pub fn fold_leaf_op(op: &OpType, consts: &[(IncomingPort, Value)]) -> ConstFoldR
            ext_op.constant_fold(consts)
        }
        _ => None,
-    }
+    };
+    assert!(fold_result.as_ref().map_or(true, |x| x.len()


should this be a debug assert?

lol, I thought that's what assert was. will change.

ss2165 · 2024-05-13T13:41:38Z

hugr/src/algorithm/const_fold.rs

+    }
+
+    /// TODO
+    pub fn run(&self, h: &mut impl HugrMut, reg: &ExtensionRegistry) -> Result<(), ConstFoldError> {


not sure I like having a run method on something with the suffix Config. There's the beginnings of a general "algorithm" interface here (configuration, transformation, verfification). Is it worth deferring some of these changes in to a generic follow up PR?

Yes, I can do that. I didn't want to through away verifying before and after the pass, but I can change that to be guarded behind #[cfg(test)].

ss2165 · 2024-05-13T13:53:10Z

hugr/src/algorithm/const_fold.rs

-            // neighbouring nodes of those added.
-            let rewrites = find_consts(h, h.nodes(), reg).collect_vec();
-            if rewrites.is_empty() {
+            // We can only safely apply a single replacement. Applying a


yes, the original code was based on the assumption that find_consts would only return rewrites for ops that only had const inputs (which would I think be safe to apply all of them?) but that's not what find_consts does

Yes I think that would be safe, if that's what find_consts did.

ss2165 · 2024-05-13T13:56:07Z

hugr/src/std_extensions/arithmetic/int_ops/const_fold.rs

@@ -16,6 +16,16 @@ use crate::{

 use super::IntOpDef;

+use lazy_static::lazy_static;
+
+lazy_static! {


I'm going to assume Alec has sufficiently reviewed the changes in this commit

ss2165

thanks!

## 🤖 New release * `hugr`: 0.3.1 -> 0.4.0 (⚠️ API breaking changes) ### ⚠️ `hugr` breaking changes ``` --- failure inherent_method_const_removed: pub method is no longer const --- Description: A publicly-visible method or associated fn is no longer `const` and can no longer be used in a `const` context. ref: https://doc.rust-lang.org/reference/const_eval.html impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.31.0/src/lints/inherent_method_const_removed.ron Failed in: ConstF64::new in /tmp/.tmpwo5blB/hugr/hugr/src/std_extensions/arithmetic/float_types.rs:43 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself 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.31.0/src/lints/struct_missing.ron Failed in: struct hugr::ops::constant::ExtensionValue, previously in file /tmp/.tmpq1W6bC/hugr/src/ops/constant.rs:184 ``` <details><summary>Changelog</summary> <blockquote> ## 0.4.0 (2024-05-20) ### Bug Fixes - Disallow non-finite values for `ConstF64` ([#1075](#1075)) - Serialization round-trips ([#948](#948)) - [**breaking**] Combine `ConstIntU` and `ConstIntS` ([#974](#974)) - Disable serialisation tests when miri is active ([#977](#977)) - [**breaking**] Serialisation schema ([#968](#968)) - Correct constant fold for `fne`. ([#995](#995)) - [**breaking**] Serialisation fixes ([#997](#997)) - [**breaking**] OpDef serialisation ([#1013](#1013)) - NaryLogicOp constant folding ([#1026](#1026)) ### Features - Add verification to constant folding ([#1030](#1030)) - Add `Const::get_custom_value` ([#1037](#1037)) - Add serialization schema for metadata ([#1038](#1038)) - 'Replace' rewrite returns node map ([#929](#929)) - `new` methods for leaf ops ([#940](#940)) - Add `string` type and `print` function to `prelude` ([#942](#942)) - `CustomOp::extension` utility function ([#951](#951)) - [**breaking**] Add `non_exhaustive` to various enums ([#952](#952)) - Encoder metadata in serialized hugr ([#955](#955)) - [**breaking**] Bring back Value ([#967](#967)) - Add LoadFunction node ([#947](#947)) - Add From impls for TypeArg ([#1002](#1002)) - Constant-folding of integer and logic operations ([#1009](#1009)) - [**breaking**] Update serialisation schema, implement `CustomConst` serialisation ([#1005](#1005)) - Merge basic blocks algorithm ([#956](#956)) - [**breaking**] Allow panic operation to have any input and output wires ([#1024](#1024)) ### Refactor - [**breaking**] Rename `crate::ops::constant::ExtensionValue` => `OpaqueValue` ([#1036](#1036)) - Outline hugr::serialize::test ([#976](#976)) - [**breaking**] Replace SmolStr identifiers with wrapper types. ([#959](#959)) - Separate extension validation from the rest ([#1011](#1011)) - Remove "trait TypeParametrised" ([#1019](#1019)) ### Testing - Reorg OutlineCfg/nest_cfgs tests so hugr doesn't depend on algorithm ([#1007](#1007)) - Ignore tests which depend on typetag when cfg(miri) ([#1051](#1051)) - Really ignore tests which depend on typetag when cfg(miri) ([#1058](#1058)) - Proptests for round trip serialisation of `Type`s and `Op`s. ([#981](#981)) - Add a test of instantiating an extension set ([#939](#939)) - Ignore serialisation tests when using miri ([#975](#975)) - [**breaking**] Test roundtrip serialisation against strict + lax schema ([#982](#982)) - Fix some bad assert_matches ([#1006](#1006)) - Expand test of instantiating extension sets ([#1003](#1003)) - Fix unconnected ports in extension test ([#1010](#1010)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Douglas Wilson <douglas.wilson@quantinuum.com>

Add validate to HugrView

6951a2e

doug-q requested review from ss2165 and cqc-alec May 13, 2024 09:41

doug-q added 3 commits May 13, 2024 10:52

Add verification to constant_fold_pass. Add a failing test

02a1a27

fix constant folding using invalid rewrites

b2c815b

fix inarrow const folding tests

862419f

doug-q force-pushed the feat/const-fold-validate branch from 83ce472 to 862419f Compare May 13, 2024 09:52

cqc-alec reviewed May 13, 2024

View reviewed changes

cqc-alec approved these changes May 13, 2024

View reviewed changes

address review

1e7a6f5

doug-q force-pushed the feat/const-fold-validate branch from b2698ea to 1e7a6f5 Compare May 13, 2024 12:45

ss2165 reviewed May 13, 2024

View reviewed changes

remove VerifyLevel etc, assert! -> debug_assert!

20b35d6

doug-q requested a review from ss2165 May 14, 2024 09:22

doug-q mentioned this pull request May 14, 2024

feat: Add ValidationLevel tooling and apply to constant_fold_pass #1035

Merged

ss2165 approved these changes May 14, 2024

View reviewed changes

doug-q added this pull request to the merge queue May 14, 2024

Merged via the queue into main with commit 81c4465 May 14, 2024
17 checks passed

doug-q deleted the feat/const-fold-validate branch May 14, 2024 10:27

This was referenced May 14, 2024

chore: release v0.4.0 #1031

Closed

chore: release v0.4.0 #1081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add verification to constant folding #1030

feat: add verification to constant folding #1030

doug-q commented May 13, 2024

doug-q commented May 13, 2024

codecov bot commented May 13, 2024 •

edited

Loading

cqc-alec May 13, 2024

cqc-alec May 13, 2024

doug-q May 13, 2024

cqc-alec May 13, 2024

doug-q May 13, 2024

cqc-alec May 13, 2024

doug-q May 13, 2024

doug-q commented May 13, 2024

ss2165 left a comment •

edited

Loading

ss2165 May 13, 2024

doug-q May 13, 2024

ss2165 May 13, 2024

doug-q May 13, 2024

ss2165 May 13, 2024

doug-q May 13, 2024

ss2165 May 13, 2024

ss2165 left a comment

feat: add verification to constant folding #1030

feat: add verification to constant folding #1030

Conversation

doug-q commented May 13, 2024

doug-q commented May 13, 2024

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q commented May 13, 2024

ss2165 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss2165 left a comment

Choose a reason for hiding this comment

codecov bot commented May 13, 2024 •

edited

Loading

ss2165 left a comment •

edited

Loading