Skip to content

Commit

Permalink
fix(fv): Add SSA pass which updates dfg's values
Browse files Browse the repository at this point in the history
We now run a SSA pass which updates the values map. We do this because
we found out that the value ids for instructions inside of fv attributes
were not matching with their expected value.

This was a bug but somehow it didn't cause us any problems until the
implementation for indexing arrays of composite types.

Now that it's fixed we removed the "hack" which was circumventing the
issue inside of the `calculate_index` function.
  • Loading branch information
Aristotelis2002 committed Jan 6, 2025
1 parent da99902 commit 5bef5b6
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 21 deletions.
1 change: 1 addition & 0 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ pub(crate) fn optimize_into_verus_vir(
.run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:")
.run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:")
.run_pass(Ssa::inline_calls_in_attributes, "After Inlining Calls in attributes:")
.run_pass(Ssa::update_value_map, "After Updating Values inside of DFGs:")
.finish();

drop(ssa_gen_span_guard);
Expand Down
4 changes: 4 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,10 @@ impl DataFlowGraph {
}
}

pub(crate) fn update_value_at_id(&mut self, value_to_replace: ValueId, new_value: Value) {
self.values[value_to_replace] = new_value;
}

/// Set the type of value_id to the target_type.
pub(crate) fn set_type_of_value(&mut self, value_id: ValueId, target_type: Type) {
let value = &mut self.values[value_id];
Expand Down
61 changes: 61 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/fv_opts/fix_value_map.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
use crate::ssa::{
ir::{
dfg::DataFlowGraph,
instruction::InstructionId,
types::Type,
value::{Value, ValueId},
},
ssa_gen::Ssa,
};

impl Ssa {
pub(crate) fn update_value_map(mut self) -> Self {
let mut values_to_update: Vec<(Vec<ValueId>, Vec<Value>)> = Vec::new();
for (_, function) in &mut self.functions {
values_to_update.clear();

for (fv_instruction_id, _) in function.dfg.get_fv_instructions_with_ids() {
let values_ids = function.dfg.instruction_results(fv_instruction_id);

let instr_as_values = transform_instruction_to_value(
fv_instruction_id,
values_ids.len(),
&function.dfg,
);

values_to_update.push((values_ids.to_vec(), instr_as_values));
}

for (values_ids, instructions) in values_to_update.iter() {
values_ids.iter().zip(instructions.iter()).for_each(|(value_id, instruction)| {
function.dfg.update_value_at_id(*value_id, instruction.clone());
});
}
}
self
}
}

fn transform_instruction_to_value(
instruction_id: InstructionId,
number_of_return_values: usize,
dfg: &DataFlowGraph,
) -> Vec<Value> {
let instruction_types: Vec<Type> = dfg
.instruction_results(instruction_id)
.iter()
.map(|val_id| dfg[*val_id].get_type().clone())
.collect();

let mut instruction_as_values: Vec<Value> = Vec::new();

for i in 0..number_of_return_values {
instruction_as_values.push(Value::Instruction {
instruction: instruction_id,
position: i,
typ: instruction_types[i].clone(),
});
}

instruction_as_values
}
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/fv_opts/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
mod fv_inline_calls;
// mod fix_value_map;
mod fix_value_map;
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,6 @@ fn call_instruction_to_expr(
/// For composite inner types we also have to calculate the index for the tuple.
fn calculate_index_and_tuple_index(
index: &ValueId,
instruction_id: &InstructionId,
inner_type_length: usize,
dfg: &DataFlowGraph,
result_id_fixer: Option<&ResultIdFixer>,
Expand All @@ -555,23 +554,8 @@ fn calculate_index_and_tuple_index(
return (ssa_value_to_expr(index, dfg, result_id_fixer), None);
}

let mut index_value = dfg[*index].clone();
// Temporary hack to circumvent the value map bug.
// We have this if check because there is a chance that dfg[index] won't return the actual value.
// This is a bug which only occurs if the array_get is located in a fv attribute.
if instruction_id.to_usize() > dfg.fv_start_id {
let previous_fv_id = InstructionId::new(instruction_id.to_usize() - 1); //Get last FV instruction
if let Some(index) = // Check if it returns the value id of the index
dfg.instruction_results(previous_fv_id).iter().position(|&res| res == *index)
{
index_value = Value::Instruction {
instruction: previous_fv_id,
position: 0,
typ: dfg.type_of_value(dfg.instruction_results(previous_fv_id)[index]),
};
}
} //TODO Delete this `if` when we fix the value map bug

let index_value = dfg[*index].clone();

// There are two possible options for the index.
// It's either a numeric constant or a Value::Instruction
match &index_value {
Expand Down Expand Up @@ -777,7 +761,6 @@ fn array_get_to_expr(
autospec_usage = AutospecUsage::IfMarked;
(index_as_vir_expr, tuple_index) = calculate_index_and_tuple_index(
index,
&instruction_id,
inner_type_length,
dfg,
current_context.result_id_fixer,
Expand All @@ -795,7 +778,6 @@ fn array_get_to_expr(
autospec_usage = AutospecUsage::Final;
(index_as_vir_expr, tuple_index) = calculate_index_and_tuple_index(
index,
&instruction_id,
inner_type_length,
dfg,
current_context.result_id_fixer,
Expand Down

0 comments on commit 5bef5b6

Please sign in to comment.