Skip to content

Commit

Permalink
bring back arrayget arrayset not pure reversion
Browse files Browse the repository at this point in the history
  • Loading branch information
vezenovm committed Apr 15, 2024
1 parent eeb5b8c commit ec175a1
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 1 deletion.
6 changes: 6 additions & 0 deletions compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 7 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(..)
Expand All @@ -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,
Expand Down
51 changes: 51 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

0 comments on commit ec175a1

Please sign in to comment.