From 87fb3c2840b921a4638dcebf103d313e7e88db6f Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 25 Nov 2024 23:26:26 +0000 Subject: [PATCH 1/5] chore: add regression test for #6620 --- .../src/ssa/opt/flatten_cfg.rs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 61a93aee58..87cd765eaa 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1463,4 +1463,53 @@ mod test { _ => unreachable!("Should have terminator instruction"), } } + + #[test] + #[should_panic] + fn correctly_handles_branches_merging_into_one_another() { + //! This is a regression test for https://github.com/noir-lang/noir/issues/6620 + + let separate_return_block_src = " + acir(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 0 at v1 + jmpif v0 then: b2, else: b1 + b2(): + jmp b3() + b1(): + store Field 2 at v1 + jmp b3() + b3(): + v5 = load v1 -> Field + constrain v5 == Field 2 + return + } + "; + let separate_ssa = Ssa::from_str(separate_return_block_src).unwrap(); + let separate_ssa = separate_ssa.flatten_cfg(); + + // This program is much the same as the above except that `b3` has been inlined into `b2`. + // This means that when we try to find the join-point of one of these branches we end up inside one of those + // branches. + let merged_return_block_src = " + acir(inline) fn main f0 { + b0(v0: u1): + v1 = allocate -> &mut Field + store Field 0 at v1 + jmpif v0 then: b2, else: b1 + b2(): + v5 = load v1 -> Field + constrain v5 == Field 2 + return + b1(): + store Field 2 at v1 + jmp b2() + } + "; + let merged_ssa = Ssa::from_str(merged_return_block_src).unwrap(); + let merged_ssa = merged_ssa.flatten_cfg(); + + assert_normalized_ssa_equals(merged_ssa, &separate_ssa.to_string()); + } } From eb51fad6a2db883132fa9c8943ab5164f15cae39 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 25 Nov 2024 23:29:32 +0000 Subject: [PATCH 2/5] Update compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 87cd765eaa..eed3238c9a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1490,8 +1490,8 @@ mod test { let separate_ssa = separate_ssa.flatten_cfg(); // This program is much the same as the above except that `b3` has been inlined into `b2`. - // This means that when we try to find the join-point of one of these branches we end up inside one of those - // branches. + // `b2` is both the `then` block and the block in which both of the branches merge together again, + // this means that we end up inlining the merge block before we've finished inlining the `else` block. let merged_return_block_src = " acir(inline) fn main f0 { b0(v0: u1): From aabf2339c6851c7e03a826ec420e30786928fcd5 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 26 Nov 2024 09:58:55 +0000 Subject: [PATCH 3/5] Update compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index eed3238c9a..e6789f48df 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1490,8 +1490,8 @@ mod test { let separate_ssa = separate_ssa.flatten_cfg(); // This program is much the same as the above except that `b3` has been inlined into `b2`. - // `b2` is both the `then` block and the block in which both of the branches merge together again, - // this means that we end up inlining the merge block before we've finished inlining the `else` block. + // `b2` is both the `then` block and the return block. This means that we don't properly + // switch to handling the `else` block by calling `then_stop` as we do in the above case. let merged_return_block_src = " acir(inline) fn main f0 { b0(v0: u1): From cdfba51917a619add7d55d7945d3f4bb3adc2dae Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 26 Nov 2024 10:12:32 +0000 Subject: [PATCH 4/5] Update compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index e6789f48df..e818e4565b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1466,7 +1466,7 @@ mod test { #[test] #[should_panic] - fn correctly_handles_branches_merging_into_one_another() { + fn correctly_handles_return_statements_in_then_block() { //! This is a regression test for https://github.com/noir-lang/noir/issues/6620 let separate_return_block_src = " From db1983c9f1918661e223f54c36576ca6a27cec91 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 26 Nov 2024 11:22:08 +0000 Subject: [PATCH 5/5] fix: panic on branches which merge within `then` branch --- .../src/ssa/opt/flatten_cfg.rs | 50 ++++++------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index e818e4565b..c18cd8c63e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -419,6 +419,16 @@ impl<'f> Context<'f> { }; self.condition_stack.push(cond_context); self.insert_current_side_effects_enabled(); + + // We disallow this case as it results in the `else_destination` block + // being inlined before the `then_destination` block due to block deduplication in the work queue. + // + // The `else_destination` block then gets treated as if it were the `then_destination` block + // and has the incorrect condition applied to it. + assert_ne!( + self.branch_ends[if_entry], *then_destination, + "ICE: branches merge inside of `then` branch" + ); vec![self.branch_ends[if_entry], *else_destination, *then_destination] } @@ -1465,51 +1475,21 @@ mod test { } #[test] - #[should_panic] - fn correctly_handles_return_statements_in_then_block() { + #[should_panic = "ICE: branches merge inside of `then` branch"] + fn panics_if_branches_merge_within_then_branch() { //! This is a regression test for https://github.com/noir-lang/noir/issues/6620 - let separate_return_block_src = " - acir(inline) fn main f0 { - b0(v0: u1): - v1 = allocate -> &mut Field - store Field 0 at v1 - jmpif v0 then: b2, else: b1 - b2(): - jmp b3() - b1(): - store Field 2 at v1 - jmp b3() - b3(): - v5 = load v1 -> Field - constrain v5 == Field 2 - return - } - "; - let separate_ssa = Ssa::from_str(separate_return_block_src).unwrap(); - let separate_ssa = separate_ssa.flatten_cfg(); - - // This program is much the same as the above except that `b3` has been inlined into `b2`. - // `b2` is both the `then` block and the return block. This means that we don't properly - // switch to handling the `else` block by calling `then_stop` as we do in the above case. - let merged_return_block_src = " + let src = " acir(inline) fn main f0 { b0(v0: u1): - v1 = allocate -> &mut Field - store Field 0 at v1 jmpif v0 then: b2, else: b1 b2(): - v5 = load v1 -> Field - constrain v5 == Field 2 return b1(): - store Field 2 at v1 jmp b2() } "; - let merged_ssa = Ssa::from_str(merged_return_block_src).unwrap(); - let merged_ssa = merged_ssa.flatten_cfg(); - - assert_normalized_ssa_equals(merged_ssa, &separate_ssa.to_string()); + let merged_ssa = Ssa::from_str(src).unwrap(); + let _ = merged_ssa.flatten_cfg(); } }