Skip to content

Commit

Permalink
fix(ssa): More robust array deduplication check (#5547)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #5529

## Summary\*

In #5287 we added a check to avoid
duplication of arrays. However, we can make this check more complete by
allowing us to check constant arrays based off of their contents when we
are inside of ACIR and also distinguishing between arrays and slices.

The program in the issue, for any size input array, is now the same
number of ACIR gates for both versions (using a local array variable and
an array fetched from a function).

## Additional Context


## 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 Jul 18, 2024
1 parent 51dd529 commit dd89b90
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 11 deletions.
40 changes: 29 additions & 11 deletions compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use iter_extended::vecmap;

use crate::ssa::ir::types::Type;

use super::{
basic_block::BasicBlockId,
dfg::{CallStack, InsertInstructionResult},
function::Function,
function::{Function, RuntimeType},
instruction::{Instruction, InstructionId},
value::ValueId,
};
Expand All @@ -16,7 +18,11 @@ pub(crate) struct FunctionInserter<'f> {
pub(crate) function: &'f mut Function,

values: HashMap<ValueId, ValueId>,
const_arrays: HashMap<im::Vector<ValueId>, ValueId>,
/// Map containing repeat array constant so that we do not initialize a new
/// array unnecessarily. An extra bool is included as part of the key to
/// distinguish between Type::Array and Type::Slice, as both are valid
/// types for a Value::Array
const_arrays: HashMap<(im::Vector<ValueId>, bool), ValueId>,
}

impl<'f> FunctionInserter<'f> {
Expand All @@ -37,15 +43,27 @@ impl<'f> FunctionInserter<'f> {
let typ = typ.clone();
let new_array: im::Vector<ValueId> =
array.iter().map(|id| self.resolve(*id)).collect();
if self.const_arrays.get(&new_array) == Some(&value) {
value
} else {
let new_array_clone = new_array.clone();
let new_id = self.function.dfg.make_array(new_array, typ);
self.values.insert(value, new_id);
self.const_arrays.insert(new_array_clone, new_id);
new_id
}

// Flag to determine the type of the value's array list
let is_array = matches!(typ, Type::Array { .. });
if let Some(fetched_value) =
self.const_arrays.get(&(new_array.clone(), is_array))
{
// Arrays in ACIR are immutable, but in Brillig arrays are copy-on-write
// so for function's with a Brillig runtime we make sure to check that value
// in our constants array map matches the resolved array value id.
if matches!(self.function.runtime(), RuntimeType::Acir(_)) {
return *fetched_value;
} else if *fetched_value == value {
return value;
}
};

let new_array_clone = new_array.clone();
let new_id = self.function.dfg.make_array(new_array, typ);
self.values.insert(value, new_id);
self.const_arrays.insert((new_array_clone, is_array), new_id);
new_id
}
_ => value,
},
Expand Down

0 comments on commit dd89b90

Please sign in to comment.