Skip to content

Commit

Permalink
fix: ArrayGet and Set are not pure (#4783)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #4600

## Summary\*

Array sets and gets are not pure. Listing them as such can cause
constant folding to fold away enabled array gets for ones in disabled
contexts (enable_side_effects_if u1 0). When used in disabled contexts
they will only ever retrieve from index zero.

## Additional Context

No reproduceable test case unfortunately. The ones in #4600 all require
aztec as a dependency and the ones in #4717 only trigger with the
remove_if_else pass added there.

I'm going to add a SSA unit test instead where two array get
instructions should survive constant folding. This PR is a draft until I
add that.

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored Apr 11, 2024
1 parent f14d913 commit 90ee479
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 90ee479

Please sign in to comment.