Skip to content

Commit

Permalink
fix(ssa): Use post order when mapping instructions in loop invariant …
Browse files Browse the repository at this point in the history
…pass (#7140)

Co-authored-by: Tom French <[email protected]>
  • Loading branch information
vezenovm and TomAFrench authored Jan 22, 2025
1 parent f73dc9a commit b00facb
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 2 deletions.
7 changes: 5 additions & 2 deletions compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::ssa::{
function::Function,
function_inserter::FunctionInserter,
instruction::{binary::eval_constant_binary_op, BinaryOp, Instruction, InstructionId},
post_order::PostOrder,
types::Type,
value::ValueId,
},
Expand Down Expand Up @@ -272,8 +273,10 @@ impl<'f> LoopInvariantContext<'f> {
/// correct new value IDs based upon the `FunctionInserter` internal map.
/// Leaving out this mapping could lead to instructions with values that do not exist.
fn map_dependent_instructions(&mut self) {
let blocks = self.inserter.function.reachable_blocks();
for block in blocks {
let mut block_order = PostOrder::with_function(self.inserter.function).into_vec();
block_order.reverse();

for block in block_order {
for instruction_id in self.inserter.function.dfg[block].take_instructions() {
self.inserter.push_instruction(instruction_id, block);
}
Expand Down
6 changes: 6 additions & 0 deletions test_programs/execution_failure/regression_7128/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "regression_7128"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
in0 = "1"
26 changes: 26 additions & 0 deletions test_programs/execution_failure/regression_7128/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
fn main(in0: Field) -> pub Field {
let mut out0: Field = 0;
let tmp1: Field = in0;

if (out0 == out0) // <== changing out0 to in0 or removing
{
// the comparison changes the result
let in0_as_bytes: [u8; 32] = in0.to_be_bytes();
let mut result: [u8; 32] = [0; 32];
for i in 0..32 {
result[i] = in0_as_bytes[i];
}
}

let mut tmp2: Field = 0; // <== moving this to the top of main,
if (0.lt(in0)) // changes the result
{
tmp2 = 1;
}

out0 = (tmp2 - tmp1);

assert(out0 != 0, "soundness violation");

out0
}
6 changes: 6 additions & 0 deletions test_programs/execution_success/regression_7128/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "regression_7128"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
in0 = "1"
26 changes: 26 additions & 0 deletions test_programs/execution_success/regression_7128/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
fn main(in0: Field) -> pub Field {
let mut out0: Field = 0;
let tmp1: Field = in0;

if (out0 == out0) // <== changing out0 to in0 or removing
{
// the comparison changes the result
let in0_as_bytes: [u8; 32] = in0.to_be_bytes();
let mut result: [u8; 32] = [0; 32];
for i in 0..32 {
result[i] = in0_as_bytes[i];
}
}

let mut tmp2: Field = 0; // <== moving this to the top of main,
if (0.lt(in0)) // changes the result
{
tmp2 = 1;
}

out0 = (tmp2 - tmp1);

assert(out0 == 0, "completeness violation");

out0
}

1 comment on commit b00facb

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: b00facb Previous: f73dc9a Ratio
regression_4709 0.002 s 0.001 s 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Please sign in to comment.