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

fix(ssa): Do not optimize for allocates in constant folding #2466

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

vezenovm
Copy link
Contributor

Description

Problem*

Quick fix found when working on (#2446) so no issue

Summary*

In order to get dynamic slices working I included a constant folding pass before flattening. In this PR (#2460) we reuse duplicate instructions with no side effects. However, Allocate instructions are not explicitly marked as having side effects so that they can be removed in DIE.

For example in execution_success/slices executing just this method:

fn merge_slices_mutate(x: Field, y: Field) -> [Field] {
    let mut slice = [0; 2];
    if x != y {
        slice = slice.push_back(y);
        slice = slice.push_back(x);
    } else {
        slice = slice.push_back(x);
    };
    slice
}

Will lead to this SSA (with constant folding before flattening):


After Mem2Reg:
acir fn main f4 {
  b0(v0: Field, v1: Field):
    v5 = allocate
    store Field 2 at v5
    v7 = allocate
    store [Field 0, Field 0] at v7
    v10 = eq v0, v1
    v11 = not v10
    jmpif v11 then: b1, else: b2
  b1():
    v21, v22 = call slice_push_back(Field 2, [Field 0, Field 0], v1)
    v25, v26 = call slice_push_back(v21, v22, v0)
    store v25 at v5
    store v26 at v7
    jmp b3()
  b3():
    v17 = load v5
    v18 = load v7
    v27 = cast v17 as u64
    v29 = lt Field 3, v27
    constrain v29
    v30 = array_get v18, index Field 3
    v32 = eq v30, Field 5
    constrain v32
    v34 = eq v17, Field 4
    constrain v34
    return 
  b2():
    v15, v16 = call slice_push_back(Field 2, [Field 0, Field 0], v0)
    store v15 at v5
    store v16 at v7
    jmp b3()
}

After Constant Folding:
acir fn main f4 {
  b0(v0: Field, v1: Field):
    v36 = allocate
    store Field 2 at v36
    store [Field 0, Field 0] at v36
    v37 = eq v0, v1
    v38 = not v37
    jmpif v38 then: b1, else: b2
  b1():
    store Field 4 at v36
    store [Field 0, Field 0, v1, v0] at v36
    jmp b3()
  b3():
    v41 = load v36
    v42 = load v36
    v43 = cast v41 as u64
    v44 = lt Field 3, v43
    constrain v44
    v45 = array_get v42, index Field 3
    v46 = eq v45, Field 5
    constrain v46
    v47 = eq v41, Field 4
    constrain v47
    return 
  b2():
    store Field 3 at v36
    store [Field 0, Field 0, v0] at v36
    jmp b3()
}

If you look in b0 the allocate instruction are being malformed. We are fetching our results from this map HashMap<Instruction, Vec> which can lead to duplicate instructions. I included a check that we do not cache allocate instructions.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@vezenovm vezenovm requested review from jfecher and TomAFrench August 28, 2023 20:31
@jfecher jfecher enabled auto-merge August 28, 2023 20:34
@jfecher jfecher added this pull request to the merge queue Aug 28, 2023
jfecher added a commit that referenced this pull request Aug 28, 2023
jfecher added a commit that referenced this pull request Aug 28, 2023
Merged via the queue into master with commit 9e272f3 Aug 28, 2023
@jfecher jfecher deleted the mv/folding-opt-fix branch August 28, 2023 21:24
TomAFrench added a commit that referenced this pull request Aug 28, 2023
* master:
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
TomAFrench added a commit that referenced this pull request Aug 30, 2023
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
TomAFrench added a commit that referenced this pull request Aug 30, 2023
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
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