Skip to content

fix: ensure that purity analysis pass explores all functions#8452

Merged
TomAFrench merged 11 commits intomasterfrom
tf/initial-post-checks
May 22, 2025
Merged

fix: ensure that purity analysis pass explores all functions#8452
TomAFrench merged 11 commits intomasterfrom
tf/initial-post-checks

Conversation

@TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented May 12, 2025

Description

Problem*

From discussion with @michaeljklein

Fixes #8625

Summary*

This PR adds some sanity checks to SSA passes so that, for instance, the SSA pass to remove bitshifts does not leave behind any bitshifts in ACIR functions.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

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

@TomAFrench TomAFrench requested a review from a team May 12, 2025 15:33
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
@TomAFrench
Copy link
Member Author

Huh, seems like we have a legit issue with purity analysis?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 98851e8 Previous: b2bfdb7 Ratio
semaphore-depth-10 0.029 s 0.021 s 1.38

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@TomAFrench
Copy link
Member Author

Here's the SSA for `array_sort` after the purity analysis as built from master with settings which panic on this branch.

After Purity Analysis (1):
brillig(inline) impure fn main f0 {
  b0(v0: [u8; 3]):
    inc_rc v0
    v1 = allocate -> &mut [u8; 3]
    store v0 at v1
    inc_rc v0
    inc_rc v0
    call f2(v1, u32 0, u32 2, Field 3)
    v6 = load v1 -> [u8; 3]
    v7 = array_get v6, index u32 0 -> u8
    constrain v7 == u8 1
    v10 = array_get v6, index u32 1 -> u8
    constrain v10 == u8 2
    v12 = array_get v6, index u32 2 -> u8
    constrain v12 == u8 3
    return
}
brillig(inline) fn cmp f1 {
  b0(v0: u8, v1: u8):
    v4 = lt v0, v1
    jmpif v4 then: b1, else: b2
  b1():
    jmp b3(Field 0)
  b2():
    v5 = lt v1, v0
    jmpif v5 then: b4, else: b5
  b3(v2: Field):
    return v2
  b4():
    jmp b6(Field 2)
  b5():
    jmp b6(Field 1)
  b6(v3: Field):
    jmp b3(v3)
}
brillig(inline) impure fn quicksort_loop f2 {
  b0(v0: &mut [u8; 3], v1: u32, v2: u32, v3: Field):
    v5 = make_array [v1, v2] : [(u32, u32)]
    v6 = allocate -> &mut u32
    store u32 1 at v6
    v8 = allocate -> &mut [(u32, u32)]
    store v5 at v8
    jmp b1(u32 0)
  b1(v4: u32):
    v11 = lt v4, u32 6
    jmpif v11 then: b2, else: b3
  b2():
    v12 = load v6 -> u32
    v13 = load v8 -> [(u32, u32)]
    inc_rc v13
    v14 = eq v12, u32 0
    jmpif v14 then: b4, else: b5
  b3():
    return
  b4():
    jmp b3()
  b5():
    v15 = load v6 -> u32
    v16 = load v8 -> [(u32, u32)]
    inc_rc v16
    v18, v19, v20, v21 = call slice_pop_back(v15, v16) -> (u32, [(u32, u32)], u32, u32)
    inc_rc v19
    store v18 at v6
    store v19 at v8
    v22 = add v20, u32 1
    v23 = lt v21, v22
    jmpif v23 then: b6, else: b7
  b6():
    v35 = unchecked_add v4, u32 1
    jmp b1(v35)
  b7():
    v25 = call f3(v0, v20, v21, v3) -> u32
    inc_rc v19
    v26 = add v25, u32 1
    v28, v29 = call slice_push_back(v18, v19, v26, v21) -> (u32, [(u32, u32)])
    store v28 at v6
    store v29 at v8
    v30 = lt u32 0, v25
    jmpif v30 then: b8, else: b9
  b8():
    inc_rc v29
    v31 = sub v25, u32 1
    v32, v33 = call slice_push_back(v28, v29, v20, v31) -> (u32, [(u32, u32)])
    store v32 at v6
    store v33 at v8
    jmp b9()
  b9():
    v34 = unchecked_add v4, u32 1
    jmp b1(v34)
}
brillig(inline) fn partition f3 {
  b0(v0: &mut [u8; 3], v1: u32, v2: u32, v3: Field):
    v5 = allocate -> &mut u32
    store v1 at v5
    jmp b1(v1)
  b1(v4: u32):
    v6 = lt v4, v2
    jmpif v6 then: b2, else: b3
  b2():
    v19 = load v0 -> [u8; 3]
    v20 = lt v4, u32 3
    constrain v20 == u1 1, "Index out of bounds"
    v21 = array_get v19, index v4 -> u8
    v22 = lt v2, u32 3
    constrain v22 == u1 1, "Index out of bounds"
    v23 = array_get v19, index v2 -> u8
    v25 = call f1(v21, v23) -> Field
    v27 = eq v25, Field 2
    v28 = not v27
    jmpif v27 then: b5, else: b4
  b3():
    v7 = load v0 -> [u8; 3]
    v8 = load v5 -> u32
    v10 = lt v8, u32 3
    constrain v10 == u1 1, "Index out of bounds"
    v12 = array_get v7, index v8 -> u8
    v13 = lt v2, u32 3
    constrain v13 == u1 1, "Index out of bounds"
    v14 = array_get v7, index v2 -> u8
    v15 = lt v8, u32 3
    constrain v15 == u1 1, "Index out of bounds"
    v16 = array_set v7, index v8, value v14
    v17 = lt v2, u32 3
    constrain v17 == u1 1, "Index out of bounds"
    v18 = array_set v16, index v2, value v12
    store v18 at v0
    return v8
  b4():
    v29 = load v0 -> [u8; 3]
    v30 = load v5 -> u32
    v31 = lt v30, u32 3
    constrain v31 == u1 1, "Index out of bounds"
    v32 = array_get v29, index v30 -> u8
    v33 = lt v4, u32 3
    constrain v33 == u1 1, "Index out of bounds"
    v34 = array_get v29, index v4 -> u8
    v35 = lt v30, u32 3
    constrain v35 == u1 1, "Index out of bounds"
    v36 = array_set v29, index v30, value v34
    v37 = lt v4, u32 3
    constrain v37 == u1 1, "Index out of bounds"
    v38 = array_set v36, index v4, value v32
    store v38 at v0
    v40 = add v30, u32 1
    store v40 at v5
    jmp b5()
  b5():
    v41 = unchecked_add v4, u32 1
    jmp b1(v41)
}

@TomAFrench TomAFrench requested a review from a team May 22, 2025 11:12
@TomAFrench TomAFrench changed the title chore: add some simple post-checks to certain SSA passes fix: ensure that purity analysis pass explores all functions May 22, 2025
@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2025

Changes to number of Brillig opcodes executed

Generated at commit: db97feee5e43f8845bc031f47558f02be8517e32, compared to commit: d992ad59d4c787aa12b0176e6f9d12202e4d9f71

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
derive_inliner_min -36 ✅ -10.26%
derive_inliner_zero -38 ✅ -13.15%

Full diff report 👇
Program Brillig opcodes (+/-) %
hashmap_inliner_min 73,232 (-103) -0.14%
slice_regex_inliner_zero 3,878 (-112) -2.81%
derive_inliner_min 315 (-36) -10.26%
derive_inliner_zero 251 (-38) -13.15%

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2025

Changes to Brillig bytecode sizes

Generated at commit: db97feee5e43f8845bc031f47558f02be8517e32, compared to commit: d992ad59d4c787aa12b0176e6f9d12202e4d9f71

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
derive_inliner_min -13 ✅ -3.63%
derive_inliner_zero -13 ✅ -4.59%

Full diff report 👇
Program Brillig opcodes (+/-) %
hashmap_inliner_min 8,749 (-11) -0.13%
slice_regex_inliner_zero 1,632 (-12) -0.73%
derive_inliner_min 345 (-13) -3.63%
derive_inliner_zero 270 (-13) -4.59%

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: b2efbfa Previous: d992ad5 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 4 s 3 s 1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@TomAFrench TomAFrench enabled auto-merge May 22, 2025 13:45
@TomAFrench
Copy link
Member Author

This is now ready for review

Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
@TomAFrench TomAFrench added this pull request to the merge queue May 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 22, 2025
@TomAFrench
Copy link
Member Author

TomAFrench commented May 22, 2025

@aakoshh Can you take all the fuzz stuff out of the merge queue CI? It seems like every other PR gets blocked on this.

@TomAFrench TomAFrench added this pull request to the merge queue May 22, 2025
Merged via the queue into master with commit 8f660a3 May 22, 2025
98 checks passed
@TomAFrench TomAFrench deleted the tf/initial-post-checks branch May 22, 2025 16:18
asterite added a commit that referenced this pull request May 27, 2025
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 1, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(fmt): correctly format mixed secondary attributes and doc comments
(noir-lang/noir#8735)
fix!: require types for trait impl associated constants
(noir-lang/noir#8734)
fix!: Prevent returning references from if expressions
(noir-lang/noir#8731)
fix: cast signed to u1 follow-up
(noir-lang/noir#8730)
fix: cast signed to u1 (noir-lang/noir#8720)
fix: do not mutate arrays later copied inside other arrays
(noir-lang/noir#8701)
chore: box `AsTraitPath` and `TypePath` in `ExpressionKind`
(noir-lang/noir#8716)
fix: general solution for accessing associated constants
(noir-lang/noir#8417)
fix(ssa): Validate checked signed add/sub is followed by a truncate
(noir-lang/noir#8706)
chore: add pre- and post-check on `array_set` optimization pass
(noir-lang/noir#8714)
fix: merge expr bindings with instantiations bindings during
monomorphization (noir-lang/noir#8713)
fix: better way to do LSP file overrides
(noir-lang/noir#8702)
fix(fmt): correct indentation when formatting long struct patterns
(noir-lang/noir#8711)
fix(fuzz): Prevent breaking/continuing out from let blocks in the AST
fuzzer (noir-lang/noir#8708)
chore: remove override for zero length inputs
(noir-lang/noir#8709)
feat: Replace converging jmpif with empty block destinations with a
single jmp (noir-lang/noir#8613)
feat(cli): Add `nargo interpret` command
(noir-lang/noir#8700)
chore: more 1-tuple printing fixes
(noir-lang/noir#8699)
chore(fuzz): Fuzz the SSA parser
(noir-lang/noir#8647)
fix: (SSA parser) translate blocks in logical order rather than syntax
order (noir-lang/noir#8668)
fix(SSA): show and parse range_check's assert_message
(noir-lang/noir#8652)
chore: Show the step number in the SSA message
(noir-lang/noir#8698)
chore(docs): Add pointers to tests
(noir-lang/noir#8695)
chore(fuzz): Capture comptime print output
(noir-lang/noir#8635)
fix: nargo expand reexports correctly implemented
(noir-lang/noir#8693)
feat(cli): Show multiple SSA passes
(noir-lang/noir#8692)
chore(test): Add regression test for defunctionalize
(noir-lang/noir#8691)
chore: add an assertion when parsing SSA that all functions are
well-formed (noir-lang/noir#8671)
feat!: prevent compiling blake3 hashes which barretenberg cannot prove
(noir-lang/noir#8690)
fix(ssa): Remove the array cache from the function inserter
(noir-lang/noir#8607)
chore: bump external pinned commits
(noir-lang/noir#8684)
chore: reenable fuzzing tests in CI
(noir-lang/noir#8688)
fix: Handle `&mut function` in defunctionalize
(noir-lang/noir#8665)
fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and
HOFs in arrays (noir-lang/noir#8672)
chore(ci): avoid running fuzzer tests in the merge queue
(noir-lang/noir#8664)
fix: Make casts in `comptime` consistent with runtime casts
(noir-lang/noir#8669)
fix: relax connectedness requirement on purity analysis pass
(noir-lang/noir#8667)
fix: always use `u32` for indexing arrays in SSA
(noir-lang/noir#8633)
chore: explicitly pull from `next` branch in aztec-packages
(noir-lang/noir#8660)
chore(fuzz): Build the dictionary from the SSA
(noir-lang/noir#8591)
chore(docs): Remove old versioned docs
(noir-lang/noir#8061)
chore: bump external pinned commits
(noir-lang/noir#8634)
fix(SSA): don't use string literal if byte is "form feed" ('\f')
(noir-lang/noir#8653)
chore(defunctionalize): Add regression test for missing lambda variants
panic (noir-lang/noir#8642)
chore(ci): Do not run ast_fuzzer orig vs. morph in ci
(noir-lang/noir#8646)
fix: disable underflow fix for fields
(noir-lang/noir#8631)
fix: revert "fix: error on unused generic in trait impl
(noir-lang/noir#8395)"
(noir-lang/noir#8636)
fix(ssa interpreter): Default to zero when we have an overflowing shl
(noir-lang/noir#8638)
fix: ensure that purity analysis pass explores all functions
(noir-lang/noir#8452)
feat: acir_formal_proofs (noir-lang/noir#8140)
fix: error on unused generic in trait impl
(noir-lang/noir#8395)
fix(expand): use re-exports for non-visibile items
(noir-lang/noir#8374)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
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.

Purity analysis pass does not analyze all functions

4 participants