feat: Push ArrayGet instructions backwards through IfElse instructions to avoid expensive array merges #5570

asterite · 2024-07-19T20:53:10Z

Description

Problem

Resolves #5501

Summary

Implements #5501

Additional Context

None.

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.

…void expensive array merges But with bad Rust

asterite · 2024-07-19T20:53:46Z

compiler/noirc_evaluator/src/ssa.rs

@@ -84,6 +84,7 @@ pub(crate) fn optimize_into_acir(
    // This pass must come immediately following `mem2reg` as the succeeding passes
    // may create an SSA which inlining fails to handle.
    .run_pass(Ssa::inline_functions_with_no_predicates, "After Inlining:")
+    .run_pass(Ssa::array_get_optimization, "After Array Get Optimizations:")


This name is temporary, we should find a name that better describes this pass.

asterite · 2024-07-19T20:54:18Z

compiler/noirc_evaluator/src/ssa/opt/array_get.rs

+            // This should match the check in flatten_cfg
+            if let crate::ssa::ir::function::RuntimeType::Brillig = function.runtime() {
+                continue;
+            }


I copied this from other passes: I'm not sure what it means though.

This just means that we only want to apply this SSA pass to functions which are being compiled to constrained ACIR rather than unconstrained Brillig. We do this as some optimisations are specific to each runtime.

This optimisation would benefit both runtimes however so we should remove this.

compiler/noirc_evaluator/src/ssa/opt/array_get.rs

TomAFrench

Quick weekend phone review.

compiler/noirc_evaluator/src/ssa/opt/array_get.rs

TomAFrench · 2024-07-20T19:37:52Z

compiler/noirc_evaluator/src/ssa/opt/array_get.rs

+            // This should match the check in flatten_cfg
+            if let crate::ssa::ir::function::RuntimeType::Brillig = function.runtime() {
+                continue;
+            }


This just means that we only want to apply this SSA pass to functions which are being compiled to constrained ACIR rather than unconstrained Brillig. We do this as some optimisations are specific to each runtime.

This optimisation would benefit both runtimes however so we should remove this.

github-actions · 2024-07-22T14:27:05Z

Changes to circuit sizes

Generated at commit: 89a775100e3c08be4ba21b9d1b04913a602b55a7, compared to commit: 1ac980bb6d227d0cc52162b0251deae92aa0ea86

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
sha256_regression	+11,411 ❌	+29.14%	+15,042 ❌	+7.41%
sha256_var_size_regression	+2,239 ❌	+11.64%	+3,101 ❌	+4.16%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
sha256_regression	50,573 (+11,411)	+29.14%	218,128 (+15,042)	+7.41%
sha256_var_size_regression	21,471 (+2,239)	+11.64%	77,700 (+3,101)	+4.16%
regression_5252	77,268 (+270)	+0.35%	84,727 (+865)	+1.03%
sha256	2,424 (+14)	+0.58%	47,628 (0)	0.00%
sha256_var_witness_const_regression	1,997 (+14)	+0.71%	44,720 (0)	0.00%

asterite · 2024-07-22T15:01:51Z

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

+        // Only if the array isn't of a tuple type (or a composite type)
+        if element_types.len() != 1 {


I'm not sure how to handle this case. Without this, insert_instruction_and_results below returns something that has multiple results and I don't know how to move that on to the next instruction. But maybe this optimization wasn't intended for composite types?

We should be able to perform this on composite types as well but we can worry about that in a follow-up. Before merging this PR we should make an issue to add this though.

asterite · 2024-07-22T17:54:50Z

I don't get that gate diff on my machine. On master:

+-----------------+----------+----------------------+--------------+
| Package         | Function | Expression Width     | ACIR Opcodes |
+-----------------+----------+----------------------+--------------+
| regression_5252 | main     | Bounded { width: 4 } | 81786        |
+-----------------+----------+----------------------+--------------+

In this PR:

+-----------------+----------+----------------------+--------------+
| Package         | Function | Expression Width     | ACIR Opcodes |
+-----------------+----------+----------------------+--------------+
| regression_5252 | main     | Bounded { width: 4 } | 81786        |
+-----------------+----------+----------------------+--------------+

vezenovm · 2024-07-22T16:58:21Z

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

+    //
+    // and a later ArrayGet instruction is this:
+    //
+    //     v11 = array_get v4, index v4


Suggested change

// v11 = array_get v4, index v4

// v11 = array_get v10, index v4

we mean v10 here?

vezenovm · 2024-07-22T17:00:37Z

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

+        };
+
+        // Don't optimize if the index is a constant (this is optimized later on in a different way)
+        if let Value::NumericConstant { .. } = &dfg[dfg.resolve(*index)] {


Suggested change

if let Value::NumericConstant { .. } = &dfg[dfg.resolve(*index)] {

if dfg.is_constant(*index) {

vezenovm · 2024-07-22T17:57:23Z

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

+        // Only if the array isn't of a tuple type (or a composite type)
+        if element_types.len() != 1 {


We should be able to perform this on composite types as well but we can worry about that in a follow-up. Before merging this PR we should make an issue to add this though.

vezenovm · 2024-07-22T17:57:32Z

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

+        //
+        // and the ArrayGet instruction is this:
+        //
+        //     v11 = array_get v4, index v4


Suggested change

// v11 = array_get v4, index v4

// v11 = array_get v10, index v4

?

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

vezenovm · 2024-07-22T18:20:18Z

In this PR:

I would make sure to run with nargo info --force as we cache build artifacts.
I got the following which is in line with the diff action:

+-----------------+----------+----------------------+--------------+
| Package         | Function | Expression Width     | ACIR Opcodes |
+-----------------+----------+----------------------+--------------+
| regression_5252 | main     | Bounded { width: 4 } | 82053        |
+-----------------+----------+----------------------+--------------+

TomAFrench · 2024-08-20T11:10:30Z

This is just blocked on the regression_5252 regression, right?

jfecher · 2024-08-20T13:31:06Z

@TomAFrench I'd say its also blocked on not showing any other circuit improvements

TomAFrench · 2024-08-20T13:39:20Z

True.

TomAFrench · 2024-08-23T13:39:47Z

I've pushed an example program which drops from 305 opcodes to 9 with this PR

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs

asterite added 3 commits July 19, 2024 17:33

Push ArrayGet instructions backwards through IfElse instructions to a…

8b2ac2a

…void expensive array merges But with bad Rust

Add some comments

9a912ca

Extract dfg variable

b4885a5

asterite commented Jul 19, 2024

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/array_get.rs Outdated Show resolved Hide resolved

asterite commented Jul 19, 2024

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/array_get.rs Outdated Show resolved Hide resolved

asterite commented Jul 19, 2024

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/array_get.rs Outdated Show resolved Hide resolved

Small comment adjustment

494b2b8

asterite commented Jul 19, 2024

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/array_get.rs Outdated Show resolved Hide resolved

TomAFrench reviewed Jul 20, 2024

View reviewed changes

asterite added 7 commits July 22, 2024 08:29

Remove extra check

50ca2ef

Fill out callstacks

539c5f9

Rename optimization

c8df51f

Add a test

660b428

No need to keep track of previous IfElse instructions

7e8b0fb

clippy

f7f8763

Don't apply optimization to array of composite types

c81bb94

Don't optimize if index is constant

7319ddb

asterite marked this pull request as ready for review July 22, 2024 15:00

asterite commented Jul 22, 2024

View reviewed changes

vezenovm reviewed Jul 22, 2024

View reviewed changes

TomAFrench and others added 2 commits August 23, 2024 13:10

Merge branch 'master' into ab/optimize-array-get-for-if-else-value

1f34c75

chore: add regression test showing large array merges

30b79de

.

1ea506f

jfecher reviewed Aug 23, 2024

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/array_get_from_if_else_result.rs Show resolved Hide resolved

Merge branch 'master' into ab/optimize-array-get-for-if-else-value

b3dfe8c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Push ArrayGet instructions backwards through IfElse instructions to avoid expensive array merges #5570

feat: Push ArrayGet instructions backwards through IfElse instructions to avoid expensive array merges #5570

asterite commented Jul 19, 2024 •

edited

Loading

asterite Jul 19, 2024 •

edited

Loading

asterite Jul 19, 2024 •

edited

Loading

TomAFrench Jul 20, 2024

TomAFrench left a comment

TomAFrench Jul 20, 2024

github-actions bot commented Jul 22, 2024 •

edited

Loading

asterite Jul 22, 2024

vezenovm Jul 22, 2024

asterite commented Jul 22, 2024

vezenovm Jul 22, 2024

vezenovm Jul 22, 2024

vezenovm Jul 22, 2024

vezenovm Jul 22, 2024

vezenovm commented Jul 22, 2024 •

edited

Loading

TomAFrench commented Aug 20, 2024

jfecher commented Aug 20, 2024

TomAFrench commented Aug 20, 2024

TomAFrench commented Aug 23, 2024

		// Only if the array isn't of a tuple type (or a composite type)
		if element_types.len() != 1 {

	// v11 = array_get v4, index v4
	// v11 = array_get v10, index v4

	if let Value::NumericConstant { .. } = &dfg[dfg.resolve(*index)] {
	if dfg.is_constant(*index) {

feat: Push ArrayGet instructions backwards through IfElse instructions to avoid expensive array merges #5570

Are you sure you want to change the base?

feat: Push ArrayGet instructions backwards through IfElse instructions to avoid expensive array merges #5570

Conversation

asterite commented Jul 19, 2024 • edited Loading

Description

Problem

Summary

Additional Context

Documentation

PR Checklist*

asterite Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

asterite Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAFrench left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2024 • edited Loading

Changes to circuit sizes

🧾 Summary (10% most significant diffs)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Jul 22, 2024

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

vezenovm commented Jul 22, 2024 • edited Loading

TomAFrench commented Aug 20, 2024

jfecher commented Aug 20, 2024

TomAFrench commented Aug 20, 2024

TomAFrench commented Aug 23, 2024

asterite commented Jul 19, 2024 •

edited

Loading

asterite Jul 19, 2024 •

edited

Loading

asterite Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024 •

edited

Loading

vezenovm commented Jul 22, 2024 •

edited

Loading