Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(ssa): Skip array_set pass for Brillig functions #6513

Merged
merged 1 commit into from
Nov 14, 2024
Merged

chore(ssa): Skip array_set pass for Brillig functions #6513

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 12 additions & 29 deletions compiler/noirc_evaluator/src/ssa/opt/array_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,21 @@

impl Function {
pub(crate) fn array_set_optimization(&mut self) {
if matches!(self.runtime(), RuntimeType::Brillig(_)) {
// Brillig is supposed to use refcounting to decide whether to mutate an array;

Check warning on line 32 in compiler/noirc_evaluator/src/ssa/opt/array_set.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (refcounting)
// array mutation was only meant for ACIR. We could use it with Brillig as well,
// but then some of the optimizations that we can do in ACIR around shared
// references have to be skipped, which makes it more cumbersome.
return;
}

let reachable_blocks = self.reachable_blocks();

if !self.runtime().is_entry_point() {
assert_eq!(reachable_blocks.len(), 1, "Expected there to be 1 block remaining in Acir function for array_set optimization");
}

let mut context = Context::new(
&self.dfg,
self.parameters(),
matches!(self.runtime(), RuntimeType::Brillig(_)),
);
let mut context = Context::new(&self.dfg);

for block in reachable_blocks.iter() {
context.analyze_last_uses(*block);
Expand All @@ -53,8 +57,6 @@

struct Context<'f> {
dfg: &'f DataFlowGraph,
function_parameters: &'f [ValueId],
is_brillig_runtime: bool,
array_to_last_use: HashMap<ValueId, InstructionId>,
instructions_that_can_be_made_mutable: HashSet<InstructionId>,
// Mapping of an array that comes from a load and whether the address
Expand All @@ -64,15 +66,9 @@
}

impl<'f> Context<'f> {
fn new(
dfg: &'f DataFlowGraph,
function_parameters: &'f [ValueId],
is_brillig_runtime: bool,
) -> Self {
fn new(dfg: &'f DataFlowGraph) -> Self {
Context {
dfg,
function_parameters,
is_brillig_runtime,
array_to_last_use: HashMap::default(),
instructions_that_can_be_made_mutable: HashSet::default(),
arrays_from_load: HashMap::default(),
Expand All @@ -94,21 +90,12 @@
self.instructions_that_can_be_made_mutable.remove(&existing);
}
}
Instruction::ArraySet { array, value, .. } => {
Instruction::ArraySet { array, .. } => {
let array = self.dfg.resolve(*array);

if let Some(existing) = self.array_to_last_use.insert(array, *instruction_id) {
self.instructions_that_can_be_made_mutable.remove(&existing);
}
if self.is_brillig_runtime {
let value = self.dfg.resolve(*value);

if let Some(existing) = self.inner_nested_arrays.get(&value) {
self.instructions_that_can_be_made_mutable.remove(existing);
}
let result = self.dfg.instruction_results(*instruction_id)[0];
self.inner_nested_arrays.insert(result, *instruction_id);
}

// If the array we are setting does not come from a load we can safely mark it mutable.
// If the array comes from a load we may potentially being mutating an array at a reference
Expand All @@ -128,17 +115,13 @@
}
});

// We cannot safely mutate slices that are inputs to the function, as they might be shared with the caller.
// NB checking the block parameters is not enough, as we might have jumped into a parameterless blocks inside the function.
let is_function_param = self.function_parameters.contains(&array);

let can_mutate = if let Some(is_from_param) = self.arrays_from_load.get(&array)
{
// If the array was loaded from a reference parameter, we cannot
// safely mark that array mutable as it may be shared by another value.
!is_from_param && is_return_block
} else {
!is_array_in_terminator && !is_function_param
!is_array_in_terminator
};

if can_mutate {
Expand Down
Loading