feat(ssa): Immediately simplify away RefCount instructions in ACIR functions #6893

aakoshh · 2024-12-20T14:22:17Z

Description

Problem*

Resolves #6831

Summary*

Assuming the runtime separation has already happened, prevents IncrementRc and DecrementRc from being added to ACIR functions by moving the runtime field from the Function to the DataFlowGrap, and modifying insert_instruction_and_results to immediately return InstructionRemoved if the Instruction would not be handled by the current runtime. This is to prevent e.g. constant_folding and loop_invariants to re-introduce these instructions, and so that we don't constantly have to keep it in mind that we should add them only in one runtime, but not the other.

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.

github-actions · 2024-12-20T14:26:51Z

Peak Memory Sample

Program	Peak Memory
keccak256	78.48M
workspace	123.64M
regression_4709	422.91M
ram_blowup_regression	1.58G
private-kernel-tail	201.60M
private-kernel-reset	716.86M
private-kernel-inner	291.68M
parity-root	171.94M

asterite

Looks good so far!

github-actions · 2024-12-20T16:01:50Z

Compilation Report

Program	Compilation Time	%
sha256_regression	1.300s	-6%
regression_4709	0.767s	-1%
ram_blowup_regression	14.760s	-2%
rollup-root	3.680s	-14%
rollup-block-merge	3.860s	6%
rollup-base-public	44.400s	12%
rollup-base-private	19.800s	-3%
private-kernel-tail	1.089s	8%
private-kernel-reset	6.758s	0%
private-kernel-inner	2.322s	11%
noir-contracts	82.600s	-10%

github-actions · 2024-12-20T16:01:51Z

Execution Report

Program	Execution Time	%
sha256_regression	0.100s	0%
regression_4709	0.001s	0%
ram_blowup_regression	0.583s	0%
rollup-root	0.126s	-2%
rollup-block-merge	0.125s	0%
rollup-base-public	2.480s	0%
rollup-base-private	1.680s	-1%
private-kernel-tail	0.023s	0%
private-kernel-reset	0.386s	-1%
private-kernel-inner	0.118s	1%

github-actions · 2025-01-03T23:28:38Z

Execution Memory Report

Program	Peak Memory
keccak256	74.61M
workspace	123.82M
regression_4709	315.93M
ram_blowup_regression	512.47M
rollup-base-public	1.18G
rollup-base-private	831.52M
private-kernel-tail	181.99M
private-kernel-reset	255.57M
private-kernel-inner	215.17M

github-actions · 2025-01-03T23:28:40Z

Compilation Memory Report

Program	Peak Memory
keccak256	78.50M
workspace	123.58M
regression_4709	422.91M
ram_blowup_regression	1.58G
rollup-base-public	2.80G
rollup-base-private	1.86G
private-kernel-tail	201.95M
private-kernel-reset	716.58M
private-kernel-inner	292.22M

TomAFrench · 2025-01-06T11:25:38Z

I think we should merge #6894 before this as we can then remove the runtime separation changes in this PR.

* master: chore: Separate unconstrained functions during monomorphization (#6894) feat!: turn CannotReexportItemWithLessVisibility into an error (#6952) feat: lock on Nargo.toml on several nargo commands (#6941)

TomAFrench

LGTM

compiler/noirc_evaluator/src/ssa/ir/dfg.rs

Co-authored-by: Maxim Vezenov <[email protected]>

This reverts commit 694e27c.

jfecher · 2025-01-06T21:05:30Z

compiler/noirc_evaluator/src/ssa/ir/dfg.rs

+        if !self.is_handled_by_runtime(&instruction) {
+            return InsertInstructionResult::InstructionRemoved;
+        }


This check shouldn't be needed anymore since #6894 already prevents inc/dec rc from being inserted into acir functions. They'd only be able to be reintroduced via at inlining optimization pass which inlines brillig code into an acir function, but that shouldn't be possible anyway.

Perhaps we'll have more brillig-only instructions in the future though? (e.g. separating array semantics)?

Also (unrelated), with runtime in the dfg now... do we really need both dfg and Function? Function now is just a wrapper around a dfg and an entry block, id, and name. IIRC we copied this from cranelift originally but I'm unsure of the motivation there. Perhaps Function had more fields. Maybe we want to avoid polluting the already large dfg with these extra fields though.

Ah that's true. I didn't notice the circuit diff disappearing from this PR.

I saw a runtime check added here, but AFAICS constant_folding and loop_invariant can still add them directly.

FWIW if I return true from is_handled_by_runtime then we get the following integration test failures because of the panic "Expected all Rc instructions to be removed before acir_gen":

tests::execution_success::test_6::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_array_dynamic_blackbox_input::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_array_to_slice::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_bigint::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_brillig_identity_function::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_conditional_regression_short_circuit::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_debug_logs::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_hashmap::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_hint_black_box::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_merkle_insert::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_nested_array_dynamic::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_pedersen_check::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_pedersen_hash::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_regression_4449::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_sha256::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_sha256_regression::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_sha256_var_padding_regression::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_sha256_var_size_regression::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_sha256_var_witness_const_regression::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_sha2_byte::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_simple_shield::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_slice_dynamic_index::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_slice_regex::forcebrillig_false_inliner_i64_min_expects tests::execution_success::test_slices::forcebrillig_false_inliner_i64_min_expects

I saw a runtime check added here, but AFAICS constant_folding and loop_invariant can still add them directly.

We probably just need to have similar runtime checks in those passes which add the rc instructions directly.

Yes, if we add the checks in the passes directly then we can roll back this entire PR. Adding them in the passes might be the only option if the runtime-specific pattern is more complicated than an individual instruction.

I did it this way because:

the ticket suggested there is value in not having instructions which will be just ignored later on, and

I thought that the passes that added them could have always been written with these checks included, so if they weren't, maybe the intention was to keep them agnostic, or at least not more complicated.

It seemed like silently skipping such instructions was the lightest touch, but I see the value in handling them in the passes as well, saving the extra checks for every instruction that is added.

…oir#6926) chore: simplify boolean in a mul of a mul (noir-lang/noir#6951) feat(ssa): Immediately simplify away RefCount instructions in ACIR functions (noir-lang/noir#6893) chore: Move comment as part of #6945 (noir-lang/noir#6959) chore: Separate unconstrained functions during monomorphization (noir-lang/noir#6894) feat!: turn CannotReexportItemWithLessVisibility into an error (noir-lang/noir#6952) feat: lock on Nargo.toml on several nargo commands (noir-lang/noir#6941) feat: don't simplify SSA instructions when creating them from a string (noir-lang/noir#6948) chore: add reproduction case for bignum test failure (noir-lang/noir#6464) chore: bump `noir-gates-diff` (noir-lang/noir#6949) feat(test): Enable the test fuzzer for Wasm (noir-lang/noir#6835) chore: also print test output to stdout in CI (noir-lang/noir#6930) fix: Non-determinism from under constrained checks (noir-lang/noir#6945) chore: use logs for benchmarking (noir-lang/noir#6911) chore: bump `noir-gates-diff` (noir-lang/noir#6944) chore: bump `noir-gates-diff` (noir-lang/noir#6943) fix: Show output of `test_program_is_idempotent` on failure (noir-lang/noir#6942) chore: delete a bunch of dead code from `noirc_evaluator` (noir-lang/noir#6939) feat: require trait function calls (`Foo::bar()`) to have the trait in scope (imported) (noir-lang/noir#6882) chore: Bump arkworks to version `0.5.0` (noir-lang/noir#6871)

chore: simplify boolean in a mul of a mul (noir-lang/noir#6951) feat(ssa): Immediately simplify away RefCount instructions in ACIR functions (noir-lang/noir#6893) chore: Move comment as part of #6945 (noir-lang/noir#6959) chore: Separate unconstrained functions during monomorphization (noir-lang/noir#6894) feat!: turn CannotReexportItemWithLessVisibility into an error (noir-lang/noir#6952) feat: lock on Nargo.toml on several nargo commands (noir-lang/noir#6941) feat: don't simplify SSA instructions when creating them from a string (noir-lang/noir#6948) chore: add reproduction case for bignum test failure (noir-lang/noir#6464) chore: bump `noir-gates-diff` (noir-lang/noir#6949) feat(test): Enable the test fuzzer for Wasm (noir-lang/noir#6835) chore: also print test output to stdout in CI (noir-lang/noir#6930) fix: Non-determinism from under constrained checks (noir-lang/noir#6945) chore: use logs for benchmarking (noir-lang/noir#6911) chore: bump `noir-gates-diff` (noir-lang/noir#6944) chore: bump `noir-gates-diff` (noir-lang/noir#6943) fix: Show output of `test_program_is_idempotent` on failure (noir-lang/noir#6942) chore: delete a bunch of dead code from `noirc_evaluator` (noir-lang/noir#6939) feat: require trait function calls (`Foo::bar()`) to have the trait in scope (imported) (noir-lang/noir#6882) chore: Bump arkworks to version `0.5.0` (noir-lang/noir#6871)

aakoshh added 3 commits December 20, 2024 10:25

Use is_brillig and is_acir

eda50c7

Remove Rc instructions after runtime separation

d317962

Move the runtime into the DFG and check during insertion

8cff5cc

aakoshh requested a review from a team December 20, 2024 14:22

aakoshh changed the title ~~6831 strip rc~~ feat(ssa): Strip increment/decrement ref-count in ACIR runtimes after separation Dec 20, 2024

asterite reviewed Dec 20, 2024

View reviewed changes

Add separated to set_runtime

e41bd9a

aakoshh and others added 2 commits December 20, 2024 16:02

Merge branch 'master' into 6831-strip-rc

6adc5f1

Merge branch 'master' into 6831-strip-rc

4648be2

TomAFrench and others added 2 commits January 3, 2025 23:45

Merge branch 'master' into 6831-strip-rc

82b1ba6

Merge branch 'master' into 6831-strip-rc

9dd079c

aakoshh requested review from TomAFrench and jfecher January 6, 2025 10:21

aakoshh and others added 5 commits January 6, 2025 15:14

Merge remote-tracking branch 'origin/master' into 6831-strip-rc

fc9ae19

Make the non-simplifying method public

a5f6157

Merge branch 'master' into 6831-strip-rc

d5a6c78

* master: chore: Separate unconstrained functions during monomorphization (#6894) feat!: turn CannotReexportItemWithLessVisibility into an error (#6952) feat: lock on Nargo.toml on several nargo commands (#6941)

.

b9598bb

.

eebf650

TomAFrench approved these changes Jan 6, 2025

View reviewed changes

aakoshh changed the title ~~feat(ssa): Strip increment/decrement ref-count in ACIR runtimes after separation~~ feat(ssa): Immediately simplify away RefCount instructions in ACIR runtimes Jan 6, 2025

vezenovm reviewed Jan 6, 2025

View reviewed changes

compiler/noirc_evaluator/src/ssa/ir/dfg.rs Outdated Show resolved Hide resolved

aakoshh changed the title ~~feat(ssa): Immediately simplify away RefCount instructions in ACIR runtimes~~ feat(ssa): Immediately simplify away RefCount instructions in ACIR functions Jan 6, 2025

aakoshh and others added 2 commits January 6, 2025 19:45

Update compiler/noirc_evaluator/src/ssa/ir/dfg.rs

694e27c

Co-authored-by: Maxim Vezenov <[email protected]>

Revert "Update compiler/noirc_evaluator/src/ssa/ir/dfg.rs"

bdb3e91

This reverts commit 694e27c.

aakoshh enabled auto-merge January 6, 2025 19:53

aakoshh added 2 commits January 6, 2025 20:11

Set runtime to Brillig in tests which expect RC instructions to be added

a7052ca

Add Default for RuntimeType

d47947d

aakoshh added this pull request to the merge queue Jan 6, 2025

Merged via the queue into master with commit ab8807d Jan 6, 2025
88 checks passed

aakoshh deleted the 6831-strip-rc branch January 6, 2025 20:35

noirwhal mentioned this pull request Jan 6, 2025

chore: Release Noir(1.0.0-beta.2) #6914

Open

jfecher reviewed Jan 6, 2025

View reviewed changes

AztecBot mentioned this pull request Jan 7, 2025

feat: Sync from noir AztecProtocol/aztec-packages#11051

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ssa): Immediately simplify away RefCount instructions in ACIR functions #6893

feat(ssa): Immediately simplify away RefCount instructions in ACIR functions #6893

aakoshh commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

asterite left a comment

github-actions bot commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Jan 3, 2025 •

edited

Loading

github-actions bot commented Jan 3, 2025 •

edited

Loading

TomAFrench commented Jan 6, 2025

TomAFrench left a comment

jfecher Jan 6, 2025

jfecher Jan 6, 2025 •

edited

Loading

TomAFrench Jan 6, 2025

aakoshh Jan 6, 2025

vezenovm Jan 7, 2025

aakoshh Jan 7, 2025 •

edited

Loading

feat(ssa): Immediately simplify away RefCount instructions in ACIR functions #6893

feat(ssa): Immediately simplify away RefCount instructions in ACIR functions #6893

Conversation

aakoshh commented Dec 20, 2024 • edited Loading

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

github-actions bot commented Dec 20, 2024 • edited Loading

Peak Memory Sample

asterite left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024 • edited Loading

Compilation Report

github-actions bot commented Dec 20, 2024 • edited Loading

Execution Report

github-actions bot commented Jan 3, 2025 • edited Loading

Execution Memory Report

github-actions bot commented Jan 3, 2025 • edited Loading

Compilation Memory Report

TomAFrench commented Jan 6, 2025

TomAFrench left a comment

Choose a reason for hiding this comment

jfecher Jan 6, 2025

Choose a reason for hiding this comment

jfecher Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

TomAFrench Jan 6, 2025

Choose a reason for hiding this comment

aakoshh Jan 6, 2025

Choose a reason for hiding this comment

vezenovm Jan 7, 2025

Choose a reason for hiding this comment

aakoshh Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

aakoshh commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Jan 3, 2025 •

edited

Loading

github-actions bot commented Jan 3, 2025 •

edited

Loading

jfecher Jan 6, 2025 •

edited

Loading

aakoshh Jan 7, 2025 •

edited

Loading