[IR] Fix setting of if merge target

When emitting the if true/false blocks we attempt to set the merge
target if the start block hasn't branched. This isn't right, as if the
block branched to other control flow, then that isn't the end of the
flow chain for that branch. We have to look at the current branch
target, and, if it exists, branch that to the if merge block.

Bug: tint:1718
Change-Id: Ifcafc4dd12c805efbee9d1dbcbc42c6add8f06a9
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/107861
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/tint/ir/builder_impl.cc

@@ -254,23 +254,18 @@ bool BuilderImpl::EmitIf(const ast::IfStatement* stmt) {
         if (!EmitStatement(stmt->body)) {
             return false;
         }
+        // If the true branch did not execute control flow, then go to the merge target
+        BranchToIfNeeded(if_node->merge_target);
 
         current_flow_block_ = if_node->false_target;
         if (stmt->else_statement && !EmitStatement(stmt->else_statement)) {
             return false;
         }
+        // If the false branch did not execute control flow, then go to the merge target
+        BranchToIfNeeded(if_node->merge_target);
     }
     current_flow_block_ = nullptr;
 
-    // If the true branch did not execute control flow, then go to the merge target
-    if (!IsBranched(if_node->true_target)) {
-        builder_.Branch(if_node->true_target, if_node->merge_target);
-    }
-    // If the false branch did not execute control flow, then go to the merge target
-    if (!IsBranched(if_node->false_target)) {
-        builder_.Branch(if_node->false_target, if_node->merge_target);
-    }
-
     // If both branches went somewhere, then they both returned, continued or broke. So,
     // there is no need for the if merge-block and there is nothing to branch to the merge
     // block anyway.
@@ -454,10 +449,6 @@ bool BuilderImpl::EmitReturn(const ast::ReturnStatement*) {
     // TODO(dsinclair): Emit the return value ....
 
     BranchTo(current_function_->end_target);
-
-    // TODO(dsinclair): The BranchTo will reset the current block. What happens with dead code
-    // after the return?
-
     return true;
 }
 
@@ -474,8 +465,6 @@ bool BuilderImpl::EmitBreak(const ast::BreakStatement*) {
         return false;
     }
 
-    // TODO(dsinclair): The BranchTo will reset the current block. What happens with dead code
-    // after the break?
     return true;
 }
 
@@ -489,9 +478,6 @@ bool BuilderImpl::EmitContinue(const ast::ContinueStatement*) {
         TINT_UNREACHABLE(IR, diagnostics_);
     }
 
-    // TODO(dsinclair): The BranchTo will reset the current block. What happens with dead code
-    // after the continue?
-
     return true;
 }
 

src/tint/ir/builder_impl_test.cc

@@ -225,6 +225,63 @@ TEST_F(IRBuilderImplTest, IfStatement_BothReturn) {
     EXPECT_EQ(flow->false_target->branch_target, func->end_target);
 }
 
+TEST_F(IRBuilderImplTest, IfStatement_JumpChainToMerge) {
+    // if (true) {
+    //   loop {
+    //     break;
+    //   }
+    // }
+    //
+    // func -> start -> if true
+    //               -> if false
+    //
+    //   [if true] -> loop
+    //   [if false] -> if merge
+    //   [if merge] -> func end
+    //   [loop] ->  loop start
+    //   [loop start] -> loop merge
+    //   [loop continuing] -> loop start
+    //   [loop merge] -> if merge
+    //
+    auto* ast_loop = Loop(Block(Break()));
+    auto* ast_if = If(true, Block(ast_loop));
+    WrapInFunction(ast_if);
+    auto& b = Build();
+
+    auto r = b.Build();
+    ASSERT_TRUE(r) << b.error();
+    auto m = r.Move();
+
+    auto* ir_if = b.FlowNodeForAstNode(ast_if);
+    ASSERT_NE(ir_if, nullptr);
+    EXPECT_TRUE(ir_if->Is<ir::If>());
+
+    auto* if_flow = ir_if->As<ir::If>();
+    ASSERT_NE(if_flow->true_target, nullptr);
+    ASSERT_NE(if_flow->false_target, nullptr);
+    ASSERT_NE(if_flow->merge_target, nullptr);
+
+    auto* ir_loop = b.FlowNodeForAstNode(ast_loop);
+    ASSERT_NE(ir_loop, nullptr);
+    EXPECT_TRUE(ir_loop->Is<ir::Loop>());
+
+    auto* loop_flow = ir_loop->As<ir::Loop>();
+    ASSERT_NE(loop_flow->start_target, nullptr);
+    ASSERT_NE(loop_flow->continuing_target, nullptr);
+    ASSERT_NE(loop_flow->merge_target, nullptr);
+
+    ASSERT_EQ(1u, m.functions.Length());
+    auto* func = m.functions[0];
+
+    EXPECT_EQ(func->start_target->branch_target, if_flow);
+    EXPECT_EQ(if_flow->true_target->branch_target, loop_flow);
+    EXPECT_EQ(loop_flow->start_target->branch_target, loop_flow->merge_target);
+    EXPECT_EQ(loop_flow->merge_target->branch_target, if_flow->merge_target);
+    EXPECT_EQ(loop_flow->continuing_target->branch_target, loop_flow->start_target);
+    EXPECT_EQ(if_flow->false_target->branch_target, if_flow->merge_target);
+    EXPECT_EQ(if_flow->merge_target->branch_target, func->end_target);
+}
+
 TEST_F(IRBuilderImplTest, Loop_WithBreak) {
     // func -> start -> loop -> loop start -> loop merge -> func end
     //