Skip to content

Commit

Permalink
feat(perf): Optimize array set from get (noir-lang#6207)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Part of general effort to reduce Brillig bytecode sizes

## Summary\*

We often times (usually when working with nested arrays) run into
patterns where we fetch an inner nested array dynamically, array set a
different array in the nested array, and the reset the other unchanged
nested array at the same index. These array sets can be optimized out.

Here is an example of what happens on master in SSA. Looking at
`nested_array_dynamic` with `--force-brillig`. Inside `b14` you will see
the following:
```
    v114 = array_get v110, index v113
    v115 = add v113, u32 1
    v116 = add v113, u32 2
    v117 = array_get v110, index v116
    v118 = array_set v110, index v113, value v114
```
After this PR `v118 = array_set v110, index v113, value v114` will
simplify to `v110`. If `v114` is unused anywhere else in the function
(which in this case it is) it will be removed by DIE.
```
    v114 = add v113, u32 1
    v115 = add v113, u32 2
    v116 = array_get v110, index v115
```

## Additional Context

Similarly to `try_optimize_array_get_from_previous_set` we should be
able to follow past `array_sets` when optimizing these `array_sets`.
e.g. if we see a pattern like this:
```
    v116 = array_get v110, index v115
    v120 = array_set v110, index v114, value [Field 100, Field 101, Field 102]
    v122 = array_set mut v120, index v115, value v116
```
We know that we performed `array_set` on the same `v110` array, however,
it was at a different index. So even though `v120` is different than
`v110` we can simplify `v122 = array_set mut v120, index v115, value
v116` to `v120`.

I am working on this in a follow-up, as the simple change in this PR
provides decent benefits on its own.

## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
vezenovm authored Oct 2, 2024
1 parent 2eb4a2c commit dfeb1c5
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use super::{
};
use acvm::acir::{brillig::MemoryAddress, AcirField};

pub(crate) const MAX_STACK_SIZE: usize = 2048;
pub(crate) const MAX_STACK_SIZE: usize = 32768;
pub(crate) const MAX_SCRATCH_SPACE: usize = 64;

impl<F: AcirField + DebugToString> BrilligContext<F, Stack> {
Expand Down
30 changes: 26 additions & 4 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,9 +630,9 @@ impl Instruction {
}
}
Instruction::ArraySet { array, index, value, .. } => {
let array = dfg.get_array_constant(*array);
let index = dfg.get_numeric_constant(*index);
if let (Some((array, element_type)), Some(index)) = (array, index) {
let array_const = dfg.get_array_constant(*array);
let index_const = dfg.get_numeric_constant(*index);
if let (Some((array, element_type)), Some(index)) = (array_const, index_const) {
let index =
index.try_to_u32().expect("Expected array index to fit in u32") as usize;

Expand All @@ -641,7 +641,8 @@ impl Instruction {
return SimplifiedTo(new_array);
}
}
None

try_optimize_array_set_from_previous_get(dfg, *array, *index, *value)
}
Instruction::Truncate { value, bit_size, max_bit_size } => {
if bit_size == max_bit_size {
Expand Down Expand Up @@ -817,6 +818,27 @@ fn try_optimize_array_get_from_previous_set(
SimplifyResult::None
}

fn try_optimize_array_set_from_previous_get(
dfg: &DataFlowGraph,
array_id: ValueId,
target_index: ValueId,
target_value: ValueId,
) -> SimplifyResult {
match &dfg[target_value] {
Value::Instruction { instruction, .. } => match &dfg[*instruction] {
Instruction::ArrayGet { array, index } => {
if *array == array_id && *index == target_index {
SimplifyResult::SimplifiedTo(array_id)
} else {
SimplifyResult::None
}
}
_ => SimplifyResult::None,
},
_ => SimplifyResult::None,
}
}

pub(crate) type ErrorType = HirType;

pub(crate) fn error_selector_from_type(typ: &ErrorType) -> ErrorSelector {
Expand Down

0 comments on commit dfeb1c5

Please sign in to comment.