From ec175a1cd740a979328c20671a3ff2388df1f286 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 15 Apr 2024 14:09:58 +0000 Subject: [PATCH] bring back arrayget arrayset not pure reversion --- .../src/ssa/function_builder/mod.rs | 6 +++ .../noirc_evaluator/src/ssa/ir/instruction.rs | 8 ++- .../src/ssa/opt/constant_folding.rs | 51 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index d3e5e506111..75a427397b6 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -326,6 +326,12 @@ impl FunctionBuilder { self.insert_instruction(Instruction::DecrementRc { value }, None); } + /// Insert an enable_side_effects_if instruction. These are normally only automatically + /// inserted during the flattening pass when branching is removed. + pub(crate) fn insert_enable_side_effects_if(&mut self, condition: ValueId) { + self.insert_instruction(Instruction::EnableSideEffects { condition }, None); + } + /// Terminates the current block with the given terminator instruction /// if the current block does not already have a terminator instruction. fn terminate_block_with(&mut self, terminator: TerminatorInstruction) { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 2b23cc1c1e8..641d971af3c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -254,7 +254,7 @@ impl Instruction { // In ACIR, a division with a false predicate outputs (0,0), so it cannot replace another instruction unless they have the same predicate bin.operator != BinaryOp::Div } - Cast(_, _) | Truncate { .. } | Not(_) | ArrayGet { .. } | ArraySet { .. } => true, + Cast(_, _) | Truncate { .. } | Not(_) => true, // These either have side-effects or interact with memory Constrain(..) @@ -266,6 +266,12 @@ impl Instruction { | DecrementRc { .. } | RangeCheck { .. } => false, + // These can have different behavior depending on the EnableSideEffectsIf context. + // Enabling constant folding for these potentially enables replacing an enabled + // array get with one that was disabled. See + // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328. + ArrayGet { .. } | ArraySet { .. } => false, + Call { func, .. } => match dfg[*func] { Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), _ => false, diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 6cac8c91bc3..5a7134f3486 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -607,4 +607,55 @@ mod test { assert_eq!(main.dfg[instructions[4]], Instruction::Constrain(v1, v_true, None)); assert_eq!(main.dfg[instructions[5]], Instruction::Constrain(v2, v_false, None)); } + + // Regression for #4600 + #[test] + fn array_get_regression() { + // fn main f0 { + // b0(v0: u1, v1: u64): + // enable_side_effects_if v0 + // v2 = array_get [Field 0, Field 1], index v1 + // v3 = not v0 + // enable_side_effects_if v3 + // v4 = array_get [Field 0, Field 1], index v1 + // } + // + // We want to make sure after constant folding both array_gets remain since they are + // under different enable_side_effects_if contexts and thus one may be disabled while + // the other is not. If one is removed, it is possible e.g. v4 is replaced with v2 which + // is disabled (only gets from index 0) and thus returns the wrong result. + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id); + let v0 = builder.add_parameter(Type::bool()); + let v1 = builder.add_parameter(Type::unsigned(64)); + + builder.insert_enable_side_effects_if(v0); + + let zero = builder.field_constant(0u128); + let one = builder.field_constant(1u128); + + let typ = Type::Array(Rc::new(vec![Type::field()]), 2); + let array = builder.array_constant(vec![zero, one].into(), typ); + + let _v2 = builder.insert_array_get(array, v1, Type::field()); + let v3 = builder.insert_not(v0); + + builder.insert_enable_side_effects_if(v3); + let _v4 = builder.insert_array_get(array, v1, Type::field()); + + // Expected output is unchanged + let ssa = builder.finish(); + let main = ssa.main(); + let instructions = main.dfg[main.entry_block()].instructions(); + let starting_instruction_count = instructions.len(); + assert_eq!(starting_instruction_count, 5); + + let ssa = ssa.fold_constants(); + let main = ssa.main(); + let instructions = main.dfg[main.entry_block()].instructions(); + let ending_instruction_count = instructions.len(); + assert_eq!(starting_instruction_count, ending_instruction_count); + } }