From da94c2b45fd43cbd6a5f2ba15b665dcb3a5e33a5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 6 Jan 2025 11:51:55 -0300 Subject: [PATCH] feat: don't simplify SSA instructions when creating them from a string (#6948) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa/function_builder/mod.rs | 26 ++++++++++++++----- .../src/ssa/ir/instruction/call.rs | 2 +- .../src/ssa/ir/instruction/call/blackbox.rs | 6 ++--- .../src/ssa/opt/constant_folding.rs | 2 +- .../src/ssa/opt/flatten_cfg.rs | 1 + .../src/ssa/opt/normalize_value_ids.rs | 13 +++++----- .../src/ssa/parser/into_ssa.rs | 11 ++++---- .../noirc_evaluator/src/ssa/parser/mod.rs | 18 +++++++++---- .../noirc_evaluator/src/ssa/parser/tests.rs | 12 +++++++++ 9 files changed, 64 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 28cf48bb171..2be8e571cd3 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -40,6 +40,10 @@ pub(crate) struct FunctionBuilder { finished_functions: Vec, call_stack: CallStackId, error_types: BTreeMap, + + /// Whether instructions are simplified as soon as they are inserted into this builder. + /// This is true by default unless changed to false after constructing a builder. + pub(crate) simplify: bool, } impl FunctionBuilder { @@ -56,6 +60,7 @@ impl FunctionBuilder { finished_functions: Vec::new(), call_stack: CallStackId::root(), error_types: BTreeMap::default(), + simplify: true, } } @@ -172,12 +177,21 @@ impl FunctionBuilder { ctrl_typevars: Option>, ) -> InsertInstructionResult { let block = self.current_block(); - self.current_function.dfg.insert_instruction_and_results( - instruction, - block, - ctrl_typevars, - self.call_stack, - ) + if self.simplify { + self.current_function.dfg.insert_instruction_and_results( + instruction, + block, + ctrl_typevars, + self.call_stack, + ) + } else { + self.current_function.dfg.insert_instruction_and_results_without_simplification( + instruction, + block, + ctrl_typevars, + self.call_stack, + ) + } } /// Switch to inserting instructions in the given block. diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index b6b1ac3f063..b0e5506e8dd 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -730,7 +730,7 @@ mod tests { return v2 } "#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); let expected = r#" brillig(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs index ffacf6fe8b5..fac2e8b4d5a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs @@ -323,7 +323,7 @@ mod test { v2 = call multi_scalar_mul (v1, v0) -> [Field; 3] return v2 }"#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); let expected_src = r#" acir(inline) fn main f0 { @@ -351,7 +351,7 @@ mod test { return v4 }"#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); //First point is zero, second scalar is zero, so we should be left with the scalar mul of the last point. let expected_src = r#" acir(inline) fn main f0 { @@ -379,7 +379,7 @@ mod test { v4 = call multi_scalar_mul (v3, v2) -> [Field; 3] return v4 }"#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); //First and last scalar/point are constant, so we should be left with the msm of the middle point and the folded constant point let expected_src = r#" acir(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index c81a557178b..e3b8c6e99aa 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1045,7 +1045,7 @@ mod test { return } "; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); let expected = " acir(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 16abf03eec0..748867c7409 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1081,6 +1081,7 @@ mod test { v23 = mul v20, Field 6 v24 = mul v21, v16 v25 = add v23, v24 + enable_side_effects v0 enable_side_effects v3 v26 = cast v3 as Field v27 = cast v0 as Field diff --git a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs index 63ca523bd57..7f5352155de 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -96,12 +96,13 @@ impl Context { .requires_ctrl_typevars() .then(|| vecmap(old_results, |result| old_function.dfg.type_of_value(*result))); - let new_results = new_function.dfg.insert_instruction_and_results( - instruction, - new_block_id, - ctrl_typevars, - new_call_stack, - ); + let new_results = + new_function.dfg.insert_instruction_and_results_without_simplification( + instruction, + new_block_id, + ctrl_typevars, + new_call_stack, + ); assert_eq!(old_results.len(), new_results.len()); for (old_result, new_result) in old_results.iter().zip(new_results.results().iter()) diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index 7c7e977c6ce..afada8c0e2c 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -16,8 +16,8 @@ use super::{ }; impl ParsedSsa { - pub(crate) fn into_ssa(self) -> Result { - Translator::translate(self) + pub(crate) fn into_ssa(self, simplify: bool) -> Result { + Translator::translate(self, simplify) } } @@ -41,8 +41,8 @@ struct Translator { } impl Translator { - fn translate(mut parsed_ssa: ParsedSsa) -> Result { - let mut translator = Self::new(&mut parsed_ssa)?; + fn translate(mut parsed_ssa: ParsedSsa, simplify: bool) -> Result { + let mut translator = Self::new(&mut parsed_ssa, simplify)?; // Note that the `new` call above removed the main function, // so all we are left with are non-main functions. @@ -53,12 +53,13 @@ impl Translator { Ok(translator.finish()) } - fn new(parsed_ssa: &mut ParsedSsa) -> Result { + fn new(parsed_ssa: &mut ParsedSsa, simplify: bool) -> Result { // A FunctionBuilder must be created with a main Function, so here wer remove it // from the parsed SSA to avoid adding it twice later on. let main_function = parsed_ssa.functions.remove(0); let main_id = FunctionId::test_new(0); let mut builder = FunctionBuilder::new(main_function.external_name.clone(), main_id); + builder.simplify = simplify; builder.set_runtime(main_function.runtime_type); // Map function names to their IDs so calls can be resolved diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index b400c28875a..96aef1f3c32 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -32,16 +32,24 @@ mod token; impl Ssa { /// Creates an Ssa object from the given string. - /// - /// Note that the resulting Ssa might not be exactly the same as the given string. - /// This is because, internally, the Ssa is built using a `FunctionBuilder`, so - /// some instructions might be simplified while they are inserted. pub(crate) fn from_str(src: &str) -> Result { + Self::from_str_impl(src, false) + } + + /// Creates an Ssa object from the given string but trying to simplify + /// each parsed instruction as it's inserted into the final SSA. + pub(crate) fn from_str_simplifying(src: &str) -> Result { + Self::from_str_impl(src, true) + } + + fn from_str_impl(src: &str, simplify: bool) -> Result { let mut parser = Parser::new(src).map_err(|err| SsaErrorWithSource::parse_error(err, src))?; let parsed_ssa = parser.parse_ssa().map_err(|err| SsaErrorWithSource::parse_error(err, src))?; - parsed_ssa.into_ssa().map_err(|error| SsaErrorWithSource { src: src.to_string(), error }) + parsed_ssa + .into_ssa(simplify) + .map_err(|error| SsaErrorWithSource { src: src.to_string(), error }) } } diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index dab96dfa04f..899a5a571e7 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -502,3 +502,15 @@ fn test_function_type() { "; assert_ssa_roundtrip(src); } + +#[test] +fn test_does_not_simplify() { + let src = " + acir(inline) fn main f0 { + b0(): + v2 = add Field 1, Field 2 + return v2 + } + "; + assert_ssa_roundtrip(src); +}