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: Respect order in bubble up for redundant asserts #4109

Merged
merged 11 commits into from
Jan 22, 2024
45 changes: 37 additions & 8 deletions compiler/noirc_evaluator/src/ssa/opt/bubble_up_constrains.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
use crate::ssa::{ir::instruction::Instruction, ssa_gen::Ssa};
use std::collections::HashMap;

use crate::ssa::{
ir::instruction::{Instruction, InstructionId},
ssa_gen::Ssa,
};

impl Ssa {
/// A simple SSA pass to go through each instruction and move every `Instruction::Constrain` to immediately
Expand All @@ -10,6 +15,11 @@ impl Ssa {
let instructions = function.dfg[block].take_instructions();
let mut filtered_instructions = Vec::with_capacity(instructions.len());

// Some insertions will be done at the same index, so we need to keep track of how many
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
// Some assertions don't operate on instruction results, so we use Option so we also track the None case
let mut inserted_at_instruction: HashMap<Option<InstructionId>, usize> =
HashMap::with_capacity(instructions.len());

let dfg = &function.dfg;
for instruction in instructions {
let (lhs, rhs) = match dfg[instruction] {
Expand All @@ -20,19 +30,38 @@ impl Ssa {
}
};

let index = filtered_instructions
let last_instruction_that_creates_inputs = filtered_instructions
.iter()
.rev()
.position(|instruction_id| {
let results = dfg.instruction_results(*instruction_id).to_vec();
.position(|&instruction_id| {
let results = dfg.instruction_results(instruction_id).to_vec();
results.contains(&lhs) || results.contains(&rhs)
})
// We iterate through the previous instructions in reverse order so the index is from the
// back of the vector. Subtract from vector length to get correct index.
.map(|reversed_index| filtered_instructions.len() - reversed_index)
.unwrap_or(0);
// back of the vector
.map(|reversed_index| filtered_instructions.len() - reversed_index - 1);

let insertion_index = last_instruction_that_creates_inputs
.map(|index| {
// We want to insert just after the last instruction that creates the inputs
index + 1
})
// If it doesn't depend from the previous instructions, then we insert at the start
.unwrap_or_default();

let already_inserted_for_this_instruction = inserted_at_instruction
.entry(
last_instruction_that_creates_inputs
.map(|index| filtered_instructions[index]),
)
.or_default();

filtered_instructions.insert(
insertion_index + *already_inserted_for_this_instruction,
instruction,
);

filtered_instructions.insert(index, instruction);
*already_inserted_for_this_instruction += 1;
}

*function.dfg[block].instructions_mut() = filtered_instructions;
Expand Down
sirasistant marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
name = "bubble_up_constrains"
type = "bin"
authors = [""]
[dependencies]
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use dep::std::field::bn254::{TWO_POW_128, assert_gt};

#[test(should_fail_with="Balance too low")]
fn test_assert_gt_should_fail_eq() {
let a: u64 = 4;
let b: u64 = 5;
assert(a >= b, "Balance too low");
assert(a >= b, "Redundant constrain");
}