From 6cad0053e2b0a525a0ba93d15387c6e0f796d81e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 13:43:08 -0600 Subject: [PATCH 1/6] Add Noop --- compiler/noirc_evaluator/src/acir/mod.rs | 1 + .../src/brillig/brillig_gen/brillig_block.rs | 1 + .../check_for_underconstrained_values.rs | 2 ++ compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 31 +++++-------------- .../noirc_evaluator/src/ssa/ir/instruction.rs | 21 ++++++++++++- .../noirc_evaluator/src/ssa/ir/printer.rs | 1 + .../src/ssa/opt/as_slice_length.rs | 7 ++--- .../src/ssa/opt/remove_enable_side_effects.rs | 1 + .../src/ssa/opt/resolve_is_unconstrained.rs | 6 ++-- 9 files changed, 39 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 95e0dd12132..a82c54d8ce6 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -788,6 +788,7 @@ impl<'a> Context<'a> { let result = dfg.instruction_results(instruction_id)[0]; self.ssa_values.insert(result, value); } + Instruction::Noop => (), } self.acir_context.set_call_stack(CallStack::new()); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 5bcddc21275..66cc213a986 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -837,6 +837,7 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.deallocate_register(items_pointer); } } + Instruction::Noop => (), }; let dead_variables = self diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 2f167ec8ab3..106b01833cd 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -365,6 +365,7 @@ impl DependencyContext { | Instruction::DecrementRc { .. } | Instruction::EnableSideEffectsIf { .. } | Instruction::IncrementRc { .. } + | Instruction::Noop | Instruction::MakeArray { .. } => {} } } @@ -626,6 +627,7 @@ impl Context { | Instruction::DecrementRc { .. } | Instruction::EnableSideEffectsIf { .. } | Instruction::IncrementRc { .. } + | Instruction::Noop | Instruction::RangeCheck { .. } => {} } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 9ccf7f11512..12520312b73 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -413,30 +413,6 @@ impl DataFlowGraph { matches!(self.values[value].get_type().as_ref(), Type::Reference(_)) } - /// Replaces an instruction result with a fresh id. - pub(crate) fn replace_result( - &mut self, - instruction_id: InstructionId, - prev_value_id: ValueId, - ) -> ValueId { - let typ = self.type_of_value(prev_value_id); - let results = self.results.get_mut(&instruction_id).unwrap(); - let res_position = results - .iter() - .position(|&id| id == prev_value_id) - .expect("Result id not found while replacing"); - - let value_id = self.values.insert(Value::Instruction { - typ, - position: res_position, - instruction: instruction_id, - }); - - // Replace the value in list of results for this instruction - results[res_position] = value_id; - value_id - } - /// Returns the number of instructions /// inserted into functions. pub(crate) fn num_instructions(&self) -> usize { @@ -448,6 +424,13 @@ impl DataFlowGraph { self.results.get(&instruction_id).expect("expected a list of Values").as_slice() } + /// Remove an instruction by replacing it with a `Noop` instruction. + /// Doing this avoids shifting over each instruction after this one in its block's instructions vector. + pub(crate) fn remove_instruction(&mut self, instruction: InstructionId) { + self.instructions[instruction] = Instruction::Noop; + self.results.remove(&instruction); + } + /// Add a parameter to the given block pub(crate) fn add_block_parameter(&mut self, block_id: BasicBlockId, typ: Type) -> ValueId { let block = &mut self.blocks[block_id]; diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 5d9bfc89f61..23d8b425349 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -335,6 +335,14 @@ pub(crate) enum Instruction { /// `typ` should be an array or slice type with an element type /// matching each of the `elements` values' types. MakeArray { elements: im::Vector, typ: Type }, + + /// A No-op instruction. These are intended to replace other instructions in a block's + /// instructions vector without having to move each instruction afterward. + /// + /// A No-op has no results and is always removed when Instruction::simplify is called. + /// When replacing another instruction, the instruction's results should always be mapped to a + /// new value since they will not be able to refer to their original instruction value any more. + Noop, } impl Instruction { @@ -360,6 +368,7 @@ impl Instruction { | Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } | Instruction::RangeCheck { .. } + | Instruction::Noop | Instruction::EnableSideEffectsIf { .. } => InstructionResultType::None, Instruction::Allocate { .. } | Instruction::Load { .. } @@ -399,7 +408,7 @@ impl Instruction { Constrain(..) | RangeCheck { .. } => true, // This should never be side-effectful - MakeArray { .. } => false, + MakeArray { .. } | Noop => false, // Some binary math can overflow or underflow Binary(binary) => match binary.operator { @@ -460,6 +469,10 @@ impl Instruction { // We can deduplicate these instructions if we know the predicate is also the same. Constrain(..) | RangeCheck { .. } => deduplicate_with_predicate, + // Noop instructions can always be deduplicated, although they're more likely to be + // removed entirely. + Noop => true, + // Arrays can be mutated in unconstrained code so code that handles this case must // take care to track whether the array was possibly mutated or not before // deduplicating. Since we don't know if the containing pass checks for this, we @@ -504,6 +517,7 @@ impl Instruction { | ArrayGet { .. } | IfElse { .. } | ArraySet { .. } + | Noop | MakeArray { .. } => true, // Store instructions must be removed by DIE in acir code, any load @@ -594,6 +608,7 @@ impl Instruction { | Instruction::IfElse { .. } | Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } + | Instruction::Noop | Instruction::MakeArray { .. } => false, } } @@ -673,6 +688,7 @@ impl Instruction { elements: elements.iter().copied().map(f).collect(), typ: typ.clone(), }, + Instruction::Noop => Instruction::Noop, } } @@ -737,6 +753,7 @@ impl Instruction { *element = f(*element); } } + Instruction::Noop => (), } } @@ -802,6 +819,7 @@ impl Instruction { f(*element); } } + Instruction::Noop => (), } } @@ -1035,6 +1053,7 @@ impl Instruction { } } Instruction::MakeArray { .. } => None, + Instruction::Noop => Remove, } } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 29e79728303..322639a03d2 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -235,6 +235,7 @@ fn display_instruction_inner( writeln!(f, "] : {typ}") } + Instruction::Noop => writeln!(f, "no-op"), } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index c6cdffd3bc3..0e7daf6c81b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -67,12 +67,11 @@ fn replace_known_slice_lengths( let call_returns = func.dfg.instruction_results(instruction_id); let original_slice_length = call_returns[0]; - // We won't use the new id for the original unknown length. - // This isn't strictly necessary as a new result will be defined the next time for which the instruction - // is reinserted but this avoids leaving the program in an invalid state. - func.dfg.replace_result(instruction_id, original_slice_length); let known_length = func.dfg.make_constant(known_length.into(), NumericType::length_type()); func.dfg.set_value_from_id(original_slice_length, known_length); + + // Now remove the original instruction + func.dfg.remove_instruction(instruction_id); }); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index e99f239e82e..a22232ba49a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -147,6 +147,7 @@ impl Context { | IfElse { .. } | IncrementRc { .. } | DecrementRc { .. } + | Noop | MakeArray { .. } => false, EnableSideEffectsIf { .. } diff --git a/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs b/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs index eff6576b87f..3ac7535a1c4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs @@ -45,11 +45,11 @@ impl Function { let call_returns = self.dfg.instruction_results(instruction_id); let original_return_id = call_returns[0]; - // We replace the result with a fresh id. This will be unused, so the DIE pass will remove the leftover intrinsic call. - self.dfg.replace_result(instruction_id, original_return_id); - // Replace all uses of the original return value with the constant self.dfg.set_value_from_id(original_return_id, is_within_unconstrained); + + // Now remove the original instruction + self.dfg.remove_instruction(instruction_id); } } } From 1b3ddd33112cff1b2a2473ef1ec884c5619388ef Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 13:45:42 -0600 Subject: [PATCH 2/6] Replace with an empty smallvec instead --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 12520312b73..9d25e792030 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -428,7 +428,7 @@ impl DataFlowGraph { /// Doing this avoids shifting over each instruction after this one in its block's instructions vector. pub(crate) fn remove_instruction(&mut self, instruction: InstructionId) { self.instructions[instruction] = Instruction::Noop; - self.results.remove(&instruction); + self.results.insert(instruction, smallvec::SmallVec::new()); } /// Add a parameter to the given block From 3bebd7f8ceddef53061898bd5cb7c231a089a6f9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 14:09:42 -0600 Subject: [PATCH 3/6] Add smallvec to cspell --- cspell.json | 1 + 1 file changed, 1 insertion(+) diff --git a/cspell.json b/cspell.json index 9a4bca1e339..826e30fa86a 100644 --- a/cspell.json +++ b/cspell.json @@ -202,6 +202,7 @@ "Secpr", "signedness", "signorecello", + "smallvec", "smol", "splitn", "srem", From d65d9b6ae49585a4f0e65c307da1c44af93bb1d7 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 14:23:17 -0600 Subject: [PATCH 4/6] Undo as_slice_length change, only the length and not the slice is mapped so the ins cant be removed --- compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs | 3 --- compiler/noirc_evaluator/src/ssa/opt/inlining.rs | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 0e7daf6c81b..09b18e087f2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -69,9 +69,6 @@ fn replace_known_slice_lengths( let known_length = func.dfg.make_constant(known_length.into(), NumericType::length_type()); func.dfg.set_value_from_id(original_slice_length, known_length); - - // Now remove the original instruction - func.dfg.remove_instruction(instruction_id); }); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 11201fc8f85..36955480728 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -458,6 +458,7 @@ impl<'function> PerFunctionContext<'function> { /// and blocks respectively. If these assertions trigger it means a value is being used before /// the instruction or block that defines the value is inserted. fn translate_value(&mut self, id: ValueId) -> ValueId { + let id = self.source_function.dfg.resolve(id); if let Some(value) = self.values.get(&id) { return *value; } From 043459def9bcb2970939bb0b3b018f323fc6a927 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 14:30:28 -0600 Subject: [PATCH 5/6] Re-add replace_result, revert as_slice_length --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 24 +++++++++++++++++++ .../src/ssa/opt/as_slice_length.rs | 5 ++++ 2 files changed, 29 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 9d25e792030..902ff0775a9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -413,6 +413,30 @@ impl DataFlowGraph { matches!(self.values[value].get_type().as_ref(), Type::Reference(_)) } + /// Replaces an instruction result with a fresh id. + pub(crate) fn replace_result( + &mut self, + instruction_id: InstructionId, + prev_value_id: ValueId, + ) -> ValueId { + let typ = self.type_of_value(prev_value_id); + let results = self.results.get_mut(&instruction_id).unwrap(); + let res_position = results + .iter() + .position(|&id| id == prev_value_id) + .expect("Result id not found while replacing"); + + let value_id = self.values.insert(Value::Instruction { + typ, + position: res_position, + instruction: instruction_id, + }); + + // Replace the value in list of results for this instruction + results[res_position] = value_id; + value_id + } + /// Returns the number of instructions /// inserted into functions. pub(crate) fn num_instructions(&self) -> usize { diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 09b18e087f2..98d5cefbda6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -67,6 +67,11 @@ fn replace_known_slice_lengths( let call_returns = func.dfg.instruction_results(instruction_id); let original_slice_length = call_returns[0]; + // We won't use the new id for the original unknown length. + // This isn't strictly necessary as a new result will be defined the next time for which the instruction + // is reinserted but this avoids leaving the program in an invalid state. + func.dfg.replace_result(instruction_id, original_slice_length); + let known_length = func.dfg.make_constant(known_length.into(), NumericType::length_type()); func.dfg.set_value_from_id(original_slice_length, known_length); }); From 023f4a76baca61400bd7fac26547cec50f0ee78d Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 14:31:01 -0600 Subject: [PATCH 6/6] Remove extra newline --- compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 98d5cefbda6..c6cdffd3bc3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -71,7 +71,6 @@ fn replace_known_slice_lengths( // This isn't strictly necessary as a new result will be defined the next time for which the instruction // is reinserted but this avoids leaving the program in an invalid state. func.dfg.replace_result(instruction_id, original_slice_length); - let known_length = func.dfg.make_constant(known_length.into(), NumericType::length_type()); func.dfg.set_value_from_id(original_slice_length, known_length); });