From ed0649dec4f1324320d4f678819eaa48e57a5aed Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Tue, 1 Nov 2022 18:17:21 +0000 Subject: [PATCH] [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 Commit-Queue: Dan Sinclair Reviewed-by: Ben Clayton --- src/tint/ir/builder_impl.cc | 22 +++--------- src/tint/ir/builder_impl_test.cc | 57 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 18 deletions(-) diff --git a/src/tint/ir/builder_impl.cc b/src/tint/ir/builder_impl.cc index 5a35b1216d..6bbcf6db89 100644 --- a/src/tint/ir/builder_impl.cc +++ b/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; } diff --git a/src/tint/ir/builder_impl_test.cc b/src/tint/ir/builder_impl_test.cc index 1e2325e21d..1ee66e57f1 100644 --- a/src/tint/ir/builder_impl_test.cc +++ b/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()); + + auto* if_flow = ir_if->As(); + 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()); + + auto* loop_flow = ir_loop->As(); + 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 //