Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ArrayGet and Set are not pure #4783

Merged
merged 3 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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. v3 is replaced with v1 which
jfecher marked this conversation as resolved.
Show resolved Hide resolved
jfecher marked this conversation as resolved.
Show resolved Hide resolved
// 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);
}
}
Loading