Skip to content

fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and HOFs in arrays#8672

Merged
jfecher merged 5 commits intomasterfrom
mv/apply-func-dynmaic-input
May 23, 2025
Merged

fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and HOFs in arrays#8672
jfecher merged 5 commits intomasterfrom
mv/apply-func-dynmaic-input

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented May 23, 2025

Description

Problem*

Working towards #5503 but not that issue.

The case this fixes has been reduced to the following:

fn main(x: u32) {
    println(lambdas_with_input_and_return_values(x));
}
fn lambdas_with_input_and_return_values(x: u32) -> u32 {
    let f1 = |value: u32| value;
    let f2 = |value: u32| value + 1;
    let f3 = |value: u32| value + 2;
    let f = if x == 0 {
        f1
    } else if x == 1 {
        f2
    } else {
        f3
    };
    f(x)
}

This is the following SSA before defunctionalization:

Details

acir(inline) fn main f0 {
  b0(v0: u32):
    v2 = call f2(v0) -> u32
    call f1(v2)
    return
}
acir(inline) fn println f1 {
  b0(v0: u32):
    call f6(u1 1, v0)
    return
}
acir(inline) fn lambdas_with_input_and_return_values f2 {
  b0(v0: u32):
    v4 = eq v0, u32 0
    jmpif v4 then: b1, else: b2
  b1():
    jmp b3(f3)
  b2():
    v6 = eq v0, u32 1
    jmpif v6 then: b4, else: b5
  b3(v1: function):
    v10 = call v1(v0) -> u32
    return v10
  b4():
    jmp b6(f4)
  b5():
    jmp b6(f5)
  b6(v2: function):
    jmp b3(v2)
}
acir(inline) fn lambda f3 {
  b0(v0: u32):
    return v0
}
acir(inline) fn lambda f4 {
  b0(v0: u32):
    v2 = add v0, u32 1
    return v2
}
acir(inline) fn lambda f5 {
  b0(v0: u32):
    v2 = add v0, u32 2
    return v2
}
brillig(inline) fn print_unconstrained f6 {
  b0(v0: u1, v1: u32):
    v21 = make_array b"{\"kind\":\"unsignedinteger\",\"width\":32}"
    call print(v0, v1, v21, u1 0)
    return
}

On master that gives the following apply function:

Details

acir(inline_always) fn apply f7 {
  b0(v0: Field, v1: u32):
    v4 = eq v0, Field 3
    jmpif v4 then: b3, else: b2
  b1(v2: u32):
    return v2
  b2():
    v8 = eq v0, Field 4
    jmpif v8 then: b5, else: b4
  b3():
    v6 = call f3(v1) -> u32
    jmp b1(v6)
  b4():
    constrain v0 == Field 5
    v13 = call f5(v1) -> u32
    jmp b1(v13)
  b5():
    v10 = call f4(v1) -> u32
    jmp b1(v10)
}

This logic panics during branch analysis:

The application panicked (crashed).
Message:  internal error: entered unreachable code: return encountered before a join point was found. This can only happen if early-return was added to the language without implementing it by jmping to a join block first
Location: compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/branch_analysis.rs:106

Follow-ups: #8674 then #8677

Summary*

@jfecher had fixed this problem for match statements here #7570. I basically copied the logic in the PR to here. I wanted to make a utility in the function builder for this code, but the one during ssa_gen uses the Tree<Value> struct that is restricted in the ssa_gen with pub(super). I felt it was simpler to duplicate code in this case. If the reviewers disagree I can work on a shared utility here. Although in general this is a workaround to handle flattening restrictionx and we can remove this code once flattening is fixed to not required to merge into one block.

This is the new apply function for the snippet above:

Details

acir(inline_always) fn apply f7 {
  b0(v0: Field, v1: u32):
    v9 = eq v0, Field 3
    jmpif v9 then: b3, else: b2
  b1(v2: u32):
    return v2
  b2():
    v13 = eq v0, Field 4
    jmpif v13 then: b6, else: b5
  b3():
    v11 = call f3(v1) -> u32
    jmp b4(v11)
  b4(v3: u32):
    jmp b10(v3)
  b5():
    constrain v0 == Field 5
    v18 = call f5(v1) -> u32
    jmp b8(v18)
  b6():
    v15 = call f4(v1) -> u32
    jmp b7(v15)
  b7(v4: u32):
    jmp b9(v4)
  b8(v5: u32):
    jmp b9(v5)
  b9(v6: u32):
    jmp b10(v6)
  b10(v7: u32):
    jmp b1(v7)
}

This is currently still a draft as I generate this larger apply function for both ACIR/Brillig runtimes. I want to see the affect on Brillig functions and if it is significant I will gate this change by runtime.
EDIT: It looks to affect the Brillig byte code by at most ~1.5%. We can gate this in the future if it starts to cause larger regressions.

Additional Context

Do we want to gate this change to ACIR runtimes? #8672 (comment)

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.

@vezenovm vezenovm marked this pull request as draft May 23, 2025 18:48
@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2025

Changes to Brillig bytecode sizes

Generated at commit: baa120804ad736c206186d4ea935beba6a1eb38b, compared to commit: b57c80e1c819b49cb88e3b948baea9c9f2e0e9ab

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
higher_order_functions_inliner_min +12 ❌ +1.08%
higher_order_functions_inliner_zero +4 ❌ +0.62%

Full diff report 👇
Program Brillig opcodes (+/-) %
higher_order_functions_inliner_min 1,122 (+12) +1.08%
higher_order_functions_inliner_zero 650 (+4) +0.62%
hashmap_inliner_min 8,789 (+40) +0.46%
uhashmap_inliner_zero 6,870 (+24) +0.35%
uhashmap_inliner_min 7,285 (+24) +0.33%
hashmap_inliner_zero 7,724 (+8) +0.10%

@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2025

Changes to number of Brillig opcodes executed

Generated at commit: baa120804ad736c206186d4ea935beba6a1eb38b, compared to commit: b57c80e1c819b49cb88e3b948baea9c9f2e0e9ab

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
higher_order_functions_inliner_min +26 ❌ +1.46%
higher_order_functions_inliner_zero +12 ❌ +1.04%

Full diff report 👇
Program Brillig opcodes (+/-) %
higher_order_functions_inliner_min 1,812 (+26) +1.46%
higher_order_functions_inliner_zero 1,163 (+12) +1.04%
hashmap_inliner_min 73,340 (+108) +0.15%
hashmap_inliner_zero 66,783 (+60) +0.09%
uhashmap_inliner_zero 168,780 (+108) +0.06%
uhashmap_inliner_min 176,694 (+108) +0.06%

@vezenovm
Copy link
Contributor Author

vezenovm commented May 23, 2025

Looks to be some minor regressions for using the new apply function vs. the one with a common return in Brillig. Do we think it is worth it to gate this change to ACIR runtimes?

@vezenovm vezenovm added the bench-show Display benchmark results on PR label May 23, 2025
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: 8b4cd72 Previous: eb3eef9 Ratio
test_report_zkpassport_noir_rsa_ 3 s 2 s 1.50

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

CC: @TomAFrench

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.

ACVM Benchmarks

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
purely_sequential_opcodes 253900 ns/iter (± 329) 267550 ns/iter (± 2599) 0.95
perfectly_parallel_opcodes 222416 ns/iter (± 475) 236248 ns/iter (± 545) 0.94
perfectly_parallel_batch_inversion_opcodes 3209303 ns/iter (± 22109) 3217535 ns/iter (± 59094) 1.00

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

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.

Execution Time

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
private-kernel-inner 0.028 s 0.028 s 1
private-kernel-reset 0.162 s 0.161 s 1.01
private-kernel-tail 0.011 s 0.011 s 1
rollup-base-private 0.312 s 0.31 s 1.01
rollup-base-public 0.198 s 0.205 s 0.97
rollup-block-root 11.6 s 11.2 s 1.04
rollup-merge 0.004 s 0.004 s 1
rollup-root 0.009 s 0.009 s 1
semaphore-depth-10 0.023 s 0.019 s 1.21

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

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.

Compilation Time

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
private-kernel-inner 2.394 s 2.47 s 0.97
private-kernel-reset 6.536 s 6.526 s 1.00
private-kernel-tail 1.154 s 1.058 s 1.09
rollup-base-private 16.26 s 16.3 s 1.00
rollup-base-public 14.74 s 14.6 s 1.01
rollup-block-root-empty 1.376 s 1.384 s 0.99
rollup-block-root-single-tx 126 s 126 s 1
rollup-block-root 127 s 126 s 1.01
rollup-merge 1.168 s 1.158 s 1.01
rollup-root 1.698 s 1.63 s 1.04
semaphore-depth-10 0.843 s 0.829 s 1.02

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

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.

Artifact Size

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
private-kernel-inner 1132.7 KB 1132.6 KB 1.00
private-kernel-reset 2051.1 KB 2051 KB 1.00
private-kernel-tail 586.6 KB 586.4 KB 1.00
rollup-base-private 5129.2 KB 5129.1 KB 1.00
rollup-base-public 4070.6 KB 4070.5 KB 1.00
rollup-block-root-empty 256.7 KB 256.7 KB 1
rollup-block-root-single-tx 25697.8 KB 25697.8 KB 1
rollup-block-root 25721.5 KB 25721.5 KB 1
rollup-merge 181.6 KB 181.6 KB 1
rollup-root 416.5 KB 416.5 KB 1
semaphore-depth-10 636.3 KB 636.3 KB 1

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

@vezenovm vezenovm marked this pull request as ready for review May 23, 2025 19:10
@vezenovm vezenovm requested a review from a team May 23, 2025 19:11
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.

Test Suite Duration

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 65 s 67 s 0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 97 s 99 s 0.98
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 45 s 44 s 1.02
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 189 s 195 s 0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 184 s 197 s 0.93
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 59 s 60 s 0.98
test_report_noir-lang_noir-bignum_ 370 s 378 s 0.98
test_report_noir-lang_noir_bigcurve_ 254 s 229 s 1.11
test_report_noir-lang_sha512_ 32 s 30 s 1.07
test_report_zkpassport_noir_rsa_ 2 s 3 s 0.67

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

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.

Compilation Memory

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
private-kernel-inner 333.01 MB 333.13 MB 1.00
private-kernel-reset 572.38 MB 572.37 MB 1.00
private-kernel-tail 232.88 MB 232.89 MB 1.00
rollup-base-private 1470 MB 1470 MB 1
rollup-base-public 1630 MB 1630 MB 1
rollup-block-root-empty 429.53 MB 429.52 MB 1.00
rollup-block-root-single-tx 7240 MB 7240 MB 1
rollup-block-root 7250 MB 7240 MB 1.00
rollup-merge 413.8 MB 413.81 MB 1.00
rollup-root 468.07 MB 468.03 MB 1.00
semaphore_depth_10 128.37 MB 128.37 MB 1

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

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.

Execution Memory

Details
Benchmark suite Current: 8274228 Previous: b57c80e Ratio
private-kernel-inner 242.05 MB 242.04 MB 1.00
private-kernel-reset 264.09 MB 264.09 MB 1
private-kernel-tail 217.23 MB 217.23 MB 1
rollup-base-private 577.89 MB 577.89 MB 1
rollup-base-public 574.21 MB 574.21 MB 1
rollup-block-root 1460 MB 1460 MB 1
rollup-merge 399.73 MB 399.73 MB 1
rollup-root 405.29 MB 405.29 MB 1
semaphore_depth_10 92.93 MB 92.93 MB 1

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

@jfecher
Copy link
Contributor

jfecher commented May 23, 2025

Looks to be some minor regressions for using the new apply function vs. the one with a common return in Brillig. Do we think it is worth it to gate this change to ACIR runtimes?

For a maximum regression of ~1.5% I think the bug fix is more important than gating it

@vezenovm
Copy link
Contributor Author

For a maximum regression of ~1.5% I think the bug fix is more important than gating it

Yeah that is what I was thinking. If larger regressions are revealed in the future we can gate it.

@vezenovm vezenovm added this pull request to the merge queue May 23, 2025
@jfecher jfecher removed this pull request from the merge queue due to a manual request May 23, 2025
@jfecher jfecher enabled auto-merge May 23, 2025 22:00
@jfecher
Copy link
Contributor

jfecher commented May 23, 2025

I just merged the 2nd PR into this one first - should be faster than merging both into master separately.

@vezenovm
Copy link
Contributor Author

I just merged the 2nd PR into this one first - should be faster than merging both into master separately.

Agreed, I missed your 2nd approval.

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: 8274228 Previous: b57c80e Ratio
semaphore-depth-10 0.023 s 0.019 s 1.21

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

CC: @TomAFrench

@vezenovm vezenovm changed the title fix(defunctionalize): Higher order functions dynamic dispatch fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and HOFs in arrays May 23, 2025
@jfecher jfecher added this pull request to the merge queue May 23, 2025
Merged via the queue into master with commit c37ded9 May 23, 2025
120 checks passed
@jfecher jfecher deleted the mv/apply-func-dynmaic-input branch May 23, 2025 22:35
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

bench-show Display benchmark results on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants