From 5bef5b630fe956739f9c2944508e8284edfddc92 Mon Sep 17 00:00:00 2001 From: Aristotelis Papanis Date: Fri, 20 Dec 2024 14:36:30 +0200 Subject: [PATCH] fix(fv): Add SSA pass which updates dfg's values 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. --- compiler/noirc_evaluator/src/ssa.rs | 1 + compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 4 ++ .../src/ssa/opt/fv_opts/fix_value_map.rs | 61 +++++++++++++++++++ .../src/ssa/opt/fv_opts/mod.rs | 2 +- .../ssa/verus_vir_gen/expr_to_vir/exprs.rs | 22 +------ 5 files changed, 69 insertions(+), 21 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/fv_opts/fix_value_map.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 044241ae6de..b6c7e96dce1 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -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); diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 9d4cc6cb492..9247bff1276 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -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]; diff --git a/compiler/noirc_evaluator/src/ssa/opt/fv_opts/fix_value_map.rs b/compiler/noirc_evaluator/src/ssa/opt/fv_opts/fix_value_map.rs new file mode 100644 index 00000000000..61df8758483 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/fv_opts/fix_value_map.rs @@ -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, Vec)> = 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 { + let instruction_types: Vec = dfg + .instruction_results(instruction_id) + .iter() + .map(|val_id| dfg[*val_id].get_type().clone()) + .collect(); + + let mut instruction_as_values: Vec = 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 +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/fv_opts/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/fv_opts/mod.rs index 343159a828f..36a97827105 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/fv_opts/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/fv_opts/mod.rs @@ -1,2 +1,2 @@ mod fv_inline_calls; -// mod fix_value_map; \ No newline at end of file +mod fix_value_map; \ No newline at end of file diff --git a/compiler/noirc_evaluator/src/ssa/verus_vir_gen/expr_to_vir/exprs.rs b/compiler/noirc_evaluator/src/ssa/verus_vir_gen/expr_to_vir/exprs.rs index e63f838c845..76c0a3ffef7 100644 --- a/compiler/noirc_evaluator/src/ssa/verus_vir_gen/expr_to_vir/exprs.rs +++ b/compiler/noirc_evaluator/src/ssa/verus_vir_gen/expr_to_vir/exprs.rs @@ -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>, @@ -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 { @@ -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, @@ -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,