Skip to content

chore: remove try_merge_only_changed_indices #8142

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

Merged
asterite merged 5 commits into from
Apr 21, 2025
Merged

chore: remove try_merge_only_changed_indices #8142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d9119fe
chore: see if not calling self.try_merge_only_changed_indices fixes it
asterite Apr 21, 2025
744d72c
Fix tests
asterite Apr 21, 2025
e1461e9
Merge branch 'master' into ab/test-do-not-use-try_merge_only_changed_…
asterite Apr 21, 2025
9495d91
Fix snaps
asterite Apr 21, 2025
4513406
Remove `try_merge_only_changed_indices`
asterite Apr 21, 2025
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
4 changes: 1 addition & 3 deletions compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,9 +505,7 @@ fn simplify_slice_push_back(
slice_sizes.insert(set_last_slice_value, slice_size / element_size);
slice_sizes.insert(new_slice, slice_size / element_size);

let unknown = &mut HashMap::default();
let mut value_merger =
ValueMerger::new(dfg, block, &mut slice_sizes, unknown, None, call_stack);
let mut value_merger = ValueMerger::new(dfg, block, &mut slice_sizes, call_stack);

let new_slice = value_merger.merge_values(
len_not_equals_capacity,
Expand Down
182 changes: 4 additions & 178 deletions compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,23 @@
use acvm::{FieldElement, acir::AcirField};
use fxhash::{FxHashMap as HashMap, FxHashSet};
use fxhash::FxHashMap as HashMap;

use crate::ssa::ir::{
basic_block::BasicBlockId,
call_stack::CallStackId,
dfg::{DataFlowGraph, InsertInstructionResult},
dfg::DataFlowGraph,
instruction::{BinaryOp, Instruction},
types::{NumericType, Type},
value::{Value, ValueId},
value::ValueId,
};

pub(crate) struct ValueMerger<'a> {
dfg: &'a mut DataFlowGraph,
block: BasicBlockId,

current_condition: Option<ValueId>,

// Maps SSA array values with a slice type to their size.
// This must be computed before merging values.
slice_sizes: &'a mut HashMap<ValueId, u32>,

array_set_conditionals: &'a mut HashMap<ValueId, ValueId>,

call_stack: CallStackId,
}

Expand All @@ -30,18 +26,9 @@ impl<'a> ValueMerger<'a> {
dfg: &'a mut DataFlowGraph,
block: BasicBlockId,
slice_sizes: &'a mut HashMap<ValueId, u32>,
array_set_conditionals: &'a mut HashMap<ValueId, ValueId>,
current_condition: Option<ValueId>,
call_stack: CallStackId,
) -> Self {
ValueMerger {
dfg,
block,
slice_sizes,
array_set_conditionals,
current_condition,
call_stack,
}
ValueMerger { dfg, block, slice_sizes, call_stack }
}

/// Merge two values a and b from separate basic blocks to a single value.
Expand Down Expand Up @@ -151,18 +138,6 @@ impl<'a> ValueMerger<'a> {
_ => panic!("Expected array type"),
};

let actual_length = len * element_types.len() as u32;

if let Some(result) = self.try_merge_only_changed_indices(
then_condition,
else_condition,
then_value,
else_value,
actual_length,
) {
return result;
}

for i in 0..len {
for (element_index, element_type) in element_types.iter().enumerate() {
let index =
Expand Down Expand Up @@ -311,153 +286,4 @@ impl<'a> ValueMerger<'a> {
}
}
}

fn try_merge_only_changed_indices(
&mut self,
then_condition: ValueId,
else_condition: ValueId,
then_value: ValueId,
else_value: ValueId,
array_length: u32,
) -> Option<ValueId> {
let mut found = false;
let current_condition = self.current_condition?;

let mut current_then = then_value;
let mut current_else = else_value;

// Arbitrarily limit this to looking at most 10 past ArraySet operations.
// If there are more than that, we assume 2 completely separate arrays are being merged.
let max_iters = 2;
let mut seen_then = Vec::with_capacity(max_iters);
let mut seen_else = Vec::with_capacity(max_iters);

// We essentially have a tree of ArraySets and want to find a common
// ancestor if it exists, alone with the path to it from each starting node.
// This path will be the indices that were changed to create each result array.
for _ in 0..max_iters {
if current_then == else_value {
seen_else.clear();
found = true;
break;
}

if current_else == then_value {
seen_then.clear();
found = true;
break;
}

if let Some(index) = seen_then.iter().position(|(elem, _, _, _)| *elem == current_else)
{
seen_else.truncate(index);
found = true;
break;
}

if let Some(index) = seen_else.iter().position(|(elem, _, _, _)| *elem == current_then)
{
seen_then.truncate(index);
found = true;
break;
}

current_then = self.find_previous_array_set(current_then, &mut seen_then);
current_else = self.find_previous_array_set(current_else, &mut seen_else);
}

let changed_indices: FxHashSet<_> = seen_then
.into_iter()
.map(|(_, index, typ, condition)| (index, typ, condition))
.chain(seen_else.into_iter().map(|(_, index, typ, condition)| (index, typ, condition)))
.collect();

if !found || changed_indices.len() as u32 >= array_length {
return None;
}

let mut array = then_value;

for (index, element_type, condition) in changed_indices {
let typevars = Some(vec![element_type.clone()]);

let instruction = Instruction::EnableSideEffectsIf { condition };
self.insert_instruction(instruction);

let mut get_element = |array, typevars| {
let get = Instruction::ArrayGet { array, index };
self.dfg
.insert_instruction_and_results(get, self.block, typevars, self.call_stack)
.first()
};

let then_element = get_element(then_value, typevars.clone());
let else_element = get_element(else_value, typevars);

let value =
self.merge_values(then_condition, else_condition, then_element, else_element);

array = self.insert_array_set(array, index, value, Some(condition)).first();
}

let instruction = Instruction::EnableSideEffectsIf { condition: current_condition };
self.insert_instruction(instruction);
Some(array)
}

fn insert_instruction(&mut self, instruction: Instruction) -> InsertInstructionResult {
self.dfg.insert_instruction_and_results(instruction, self.block, None, self.call_stack)
}

fn insert_array_set(
&mut self,
array: ValueId,
index: ValueId,
value: ValueId,
condition: Option<ValueId>,
) -> InsertInstructionResult {
let instruction = Instruction::ArraySet { array, index, value, mutable: false };
let result =
self.dfg.insert_instruction_and_results(instruction, self.block, None, self.call_stack);

if let Some(condition) = condition {
let result_index = if result.len() == 1 {
0
} else {
// Slices return (length, slice)
assert_eq!(result.len(), 2);
1
};

let result_value = result[result_index];
self.array_set_conditionals.insert(result_value, condition);
}

result
}

fn find_previous_array_set(
&self,
result: ValueId,
changed_indices: &mut Vec<(ValueId, ValueId, Type, ValueId)>,
) -> ValueId {
match &self.dfg[result] {
Value::Instruction { instruction, .. } => match &self.dfg[*instruction] {
Instruction::ArraySet { array, index, value, .. } => {
let condition =
*self.array_set_conditionals.get(&result).unwrap_or_else(|| {
panic!(
"Expected to have conditional for array set {result}\n{:?}",
self.array_set_conditionals
)
});
let element_type = self.dfg.type_of_value(*value);
changed_indices.push((result, *index, element_type, condition));
*array
}
_ => result,
},
_ => result,
}
}
}
59 changes: 22 additions & 37 deletions compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
use std::collections::hash_map::Entry;

use acvm::{FieldElement, acir::AcirField};
use fxhash::FxHashMap as HashMap;

use crate::ssa::ir::function::RuntimeType;
use crate::ssa::ir::instruction::Hint;
use crate::ssa::ir::types::NumericType;
use crate::ssa::ir::value::ValueId;
use crate::ssa::{
Ssa,
Expand Down Expand Up @@ -51,9 +49,6 @@ impl Function {
#[derive(Default)]
struct Context {
slice_sizes: HashMap<ValueId, u32>,

// Maps array_set result -> enable_side_effects_if value which was active during it.
array_set_conditionals: HashMap<ValueId, ValueId>,
}

impl Context {
Expand All @@ -63,9 +58,6 @@ impl Context {
// Make sure this optimization runs when there's only one block
assert_eq!(function.dfg[block].successors().count(), 0);

let one = FieldElement::one();
let mut current_conditional = function.dfg.make_constant(one, NumericType::bool());

function.simple_reachable_blocks_optimization(|context| {
let instruction_id = context.instruction_id;
let instruction = context.instruction();
Expand All @@ -81,14 +73,8 @@ impl Context {
assert!(!matches!(typ, Type::Numeric(_)));

let call_stack = context.dfg.get_instruction_call_stack_id(instruction_id);
let mut value_merger = ValueMerger::new(
context.dfg,
block,
&mut self.slice_sizes,
&mut self.array_set_conditionals,
Some(current_conditional),
call_stack,
);
let mut value_merger =
ValueMerger::new(context.dfg, block, &mut self.slice_sizes, call_stack);

let value = value_merger.merge_values(
then_condition,
Expand All @@ -108,7 +94,6 @@ impl Context {

context.remove_current_instruction();
context.replace_value(result, value);
self.array_set_conditionals.insert(value, current_conditional);
}
Instruction::Call { func, arguments } => {
if let Value::Intrinsic(intrinsic) = context.dfg[*func] {
Expand Down Expand Up @@ -136,14 +121,9 @@ impl Context {
let results = context.dfg.instruction_results(instruction_id);
let result = if results.len() == 2 { results[1] } else { results[0] };

self.array_set_conditionals.insert(result, current_conditional);

let old_capacity = self.get_or_find_capacity(context.dfg, *array);
self.slice_sizes.insert(result, old_capacity);
}
Instruction::EnableSideEffectsIf { condition } => {
current_conditional = *condition;
}
_ => (),
}
});
Expand Down Expand Up @@ -357,29 +337,34 @@ mod tests {
// rather than merging the entire array. As we only modify the `y` array at a single index,
// we instead only map the if predicate onto the the numeric value we are looking to write,
// and then write into the array directly.
assert_ssa_snapshot!(ssa, @r#"
assert_ssa_snapshot!(ssa, @r"
acir(inline) predicate_pure fn main f0 {
b0(v0: u1, v1: [u32; 2]):
v2 = allocate -> &mut [u32; 2]
enable_side_effects v0
v5 = array_set v1, index u32 0, value u32 1
v6 = not v0
enable_side_effects v0
v7 = array_get v1, index u32 0 -> u32
v8 = cast v0 as u32
v9 = cast v6 as u32
v10 = unchecked_mul v9, v7
v11 = unchecked_add v8, v10
v12 = array_set v5, index u32 0, value v11
enable_side_effects v0
v8 = array_get v1, index Field 0 -> u32
v9 = cast v0 as u32
v10 = cast v6 as u32
v11 = unchecked_mul v10, v8
v12 = unchecked_add v9, v11
v14 = array_get v5, index Field 1 -> u32
v15 = array_get v1, index Field 1 -> u32
v16 = cast v0 as u32
v17 = cast v6 as u32
v18 = unchecked_mul v16, v14
v19 = unchecked_mul v17, v15
v20 = unchecked_add v18, v19
v21 = make_array [v12, v20] : [u32; 2]
enable_side_effects u1 1
v14 = array_get v12, index u32 0 -> u32
v15 = array_get v12, index u32 1 -> u32
v16 = add v14, v15
v17 = eq v16, u32 1
constrain v16 == u32 1
v23 = array_get v21, index u32 0 -> u32
v24 = array_get v21, index u32 1 -> u32
v25 = add v23, v24
v26 = eq v25, u32 1
constrain v25 == u32 1
return
}
"#);
");
}
}
Loading