Skip to content

Commit

Permalink
fix: Fix brillig slowdown when assigning arrays in loops (#4472)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #3795

## Summary\*

This is an older version of #4210
which undoes the change in
d331ee2
due to a regression #4332.

This PR is not yet confirmed to work since I do not have a test case for
it! @sirasistant, do you mind seeing if this fixes the regression issue?

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored Mar 11, 2024
1 parent 8d66677 commit 2a53545
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 2 deletions.
19 changes: 17 additions & 2 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,10 +393,25 @@ impl FunctionBuilder {
self.increment_array_reference_count(value);
}
}
Type::Array(..) | Type::Slice(..) => {
self.insert_instruction(Instruction::IncrementRc { value }, None);
typ @ Type::Array(..) | typ @ Type::Slice(..) => {
// If there are nested arrays or slices, we wait until ArrayGet
// is issued to increment the count of that array.
self.insert_instruction(Instruction::IncrementRc { value }, None);

// This is a bit odd, but in brillig the inc_rc instruction operates on
// a copy of the array's metadata, so we need to re-store a loaded array
// even if there have been no other changes to it.
if let Value::Instruction { instruction, .. } = &self.current_function.dfg[value] {
let instruction = &self.current_function.dfg[*instruction];
if let Instruction::Load { address } = instruction {
// We can't re-use `value` in case the original address was stored
// to again in the meantime. So introduce another load.
let address = *address;
let value = self.insert_load(address, typ);
self.insert_instruction(Instruction::IncrementRc { value }, None);
self.insert_store(address, value);
}
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,11 @@ impl<'a> FunctionContext<'a> {
let lhs = self.extract_current_value(&assign.lvalue)?;
let rhs = self.codegen_expression(&assign.expression)?;

rhs.clone().for_each(|value| {
let value = value.eval(self);
self.builder.increment_array_reference_count(value);
});

self.assign_new_value(lhs, rhs);
Ok(Self::unit_value())
}
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/brillig_cow_assign/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "brillig_cow_assign"
type = "bin"
authors = [""]
compiler_version = ">=0.23.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
items_to_update = 10
index = 6
23 changes: 23 additions & 0 deletions test_programs/execution_success/brillig_cow_assign/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
global N = 10;

unconstrained fn main() {
let mut arr = [0; N];
let mut mid_change = arr;

for i in 0..N {
if i == N / 2 {
mid_change = arr;
}
arr[i] = 27;
}

// Expect:
// arr = [27, 27, 27, 27, 27, 27, 27, 27, 27, 27]
// mid_change = [27, 27, 27, 27, 27, 0, 0, 0, 0, 0]

let modified_i = N / 2 + 1;
assert_eq(arr[modified_i], 27);

// Fail here!
assert(mid_change[modified_i] != 27);
}

0 comments on commit 2a53545

Please sign in to comment.