[spirv-writer] Avoid branch after dead if/then if/else

Bug: tint:64
Change-Id: I008c449ca634c6410055a65927199fda2d7bbb06
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/20720
Reviewed-by: dan sinclair <dsinclair@google.com>

src/writer/spirv/builder.cc

@@ -94,6 +94,8 @@ bool LastIsTerminator(const ast::StatementList& stmts) {
   }
 
   auto* last = stmts.back().get();
+  // TODO(dneto): Conditional break and conditional continue should return
+  // false.
   return last->IsBreak() || last->IsContinue() || last->IsReturn() ||
          last->IsKill() || last->IsFallthrough();
 }
@@ -1027,9 +1029,10 @@ bool Builder::GenerateConditionalBlock(
   if (!GenerateStatementList(true_body)) {
     return false;
   }
-  // TODO(dsinclair): The branch should be optional based on how the
+  // We only branch if the last element of the body didn't already branch.
-  // StatementList ended ...
+  if (!LastIsTerminator(true_body)) {
-  push_function_inst(spv::Op::OpBranch, {Operand::Int(merge_block_id)});
+    push_function_inst(spv::Op::OpBranch, {Operand::Int(merge_block_id)});
+  }
 
   // Start the false block if needed
   if (false_block_id != merge_block_id) {
@@ -1041,14 +1044,13 @@ bool Builder::GenerateConditionalBlock(
       if (!GenerateStatementList(else_stmt->body())) {
         return false;
       }
-      // TODO(dsinclair): The branch should be optional based on how the
-      // StatementList ended ...
-      push_function_inst(spv::Op::OpBranch, {Operand::Int(merge_block_id)});
     } else {
       if (!GenerateConditionalBlock(else_stmt->condition(), else_stmt->body(),
                                     cur_else_idx + 1, else_stmts)) {
         return false;
       }
+    }
+    if (!LastIsTerminator(else_stmt->body())) {
       push_function_inst(spv::Op::OpBranch, {Operand::Int(merge_block_id)});
     }
   }

src/writer/spirv/builder_if_test.cc

@@ -17,11 +17,16 @@
 #include "gtest/gtest.h"
 #include "src/ast/assignment_statement.h"
 #include "src/ast/bool_literal.h"
+#include "src/ast/break_statement.h"
+#include "src/ast/continue_statement.h"
 #include "src/ast/else_statement.h"
 #include "src/ast/identifier_expression.h"
 #include "src/ast/if_statement.h"
 #include "src/ast/int_literal.h"
+#include "src/ast/loop_statement.h"
+#include "src/ast/return_statement.h"
 #include "src/ast/scalar_constructor_expression.h"
+#include "src/ast/statement_condition.h"
 #include "src/ast/type/bool_type.h"
 #include "src/ast/type/i32_type.h"
 #include "src/context.h"
@@ -367,22 +372,313 @@ OpBranch %6
 )");
 }
 
-TEST_F(BuilderTest, DISABLED_If_WithBreak) {
+TEST_F(BuilderTest, If_WithBreak) {
-  // if (a) {
+  ast::type::BoolType bool_type;
-  //   break;
+  // loop {
+  //   if (true) {
+  //     break;
+  //   }
   // }
+  auto cond = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, true));
+
+  ast::StatementList if_body;
+  if_body.push_back(std::make_unique<ast::BreakStatement>());
+
+  auto if_stmt =
+      std::make_unique<ast::IfStatement>(std::move(cond), std::move(if_body));
+
+  ast::StatementList loop_body;
+  loop_body.push_back(std::move(if_stmt));
+
+  ast::LoopStatement expr(std::move(loop_body), {});
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+
+  EXPECT_TRUE(b.GenerateLoopStatement(&expr)) << b.error();
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%5 = OpTypeBool
+%6 = OpConstantTrue %5
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpBranch %1
+%1 = OpLabel
+OpLoopMerge %2 %3 None
+OpBranch %4
+%4 = OpLabel
+OpSelectionMerge %7 None
+OpBranchConditional %6 %8 %7
+%8 = OpLabel
+OpBranch %2
+%7 = OpLabel
+OpBranch %3
+%3 = OpLabel
+OpBranch %1
+%2 = OpLabel
+)");
 }
 
-TEST_F(BuilderTest, DISABLED_If_WithContinue) {
+TEST_F(BuilderTest, If_WithElseBreak) {
-  // if (a) {
+  ast::type::BoolType bool_type;
-  //   continue;
+  // loop {
+  //   if (true) {
+  //   } else {
+  //     break;
+  //   }
   // }
+  auto cond = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, true));
+
+  ast::StatementList if_body;
+  ast::StatementList else_body;
+  else_body.push_back(std::make_unique<ast::BreakStatement>());
+
+  ast::ElseStatementList else_stmts;
+  else_stmts.push_back(
+      std::make_unique<ast::ElseStatement>(std::move(else_body)));
+
+  auto if_stmt =
+      std::make_unique<ast::IfStatement>(std::move(cond), std::move(if_body));
+  if_stmt->set_else_statements(std::move(else_stmts));
+
+  ast::StatementList loop_body;
+  loop_body.push_back(std::move(if_stmt));
+
+  ast::LoopStatement expr(std::move(loop_body), {});
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+
+  EXPECT_TRUE(b.GenerateLoopStatement(&expr)) << b.error();
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%5 = OpTypeBool
+%6 = OpConstantTrue %5
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpBranch %1
+%1 = OpLabel
+OpLoopMerge %2 %3 None
+OpBranch %4
+%4 = OpLabel
+OpSelectionMerge %7 None
+OpBranchConditional %6 %8 %9
+%8 = OpLabel
+OpBranch %7
+%9 = OpLabel
+OpBranch %2
+%7 = OpLabel
+OpBranch %3
+%3 = OpLabel
+OpBranch %1
+%2 = OpLabel
+)");
 }
 
-TEST_F(BuilderTest, DISABLED_IF_WithReturn) {
+// This is blocked on implementing conditional break
-  // if (a) {
+TEST_F(BuilderTest, DISABLED_If_WithConditionalBreak) {
+  ast::type::BoolType bool_type;
+  // loop {
+  //   if (true) {
+  //     break if (false);
+  //   }
+  // }
+  auto cond_true = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, true));
+  auto cond_false = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, false));
+
+  ast::StatementList if_body;
+  if_body.push_back(std::make_unique<ast::BreakStatement>(
+      ast::StatementCondition::kIf, std::move(cond_false)));
+
+  auto if_stmt = std::make_unique<ast::IfStatement>(std::move(cond_true),
+                                                    std::move(if_body));
+
+  ast::StatementList loop_body;
+  loop_body.push_back(std::move(if_stmt));
+
+  ast::LoopStatement expr(std::move(loop_body), {});
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+
+  EXPECT_TRUE(b.GenerateLoopStatement(&expr)) << b.error();
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%5 = OpTypeBool
+%6 = OpConstantTrue %5
+%7 = OpConstantFalse %5
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpBranch %1
+%1 = OpLabel
+OpLoopMerge %2 %3 None
+OpBranch %4
+%4 = OpLabel
+OpSelectionMerge %8 None
+OpBranchConditional %6 %9 %8
+%9 = OpLabel
+OpBranchConditional %7 %2 %8
+%8 = OpLabel
+OpBranch %3
+%3 = OpLabel
+OpBranch %1
+%2 = OpLabel
+)");
+}
+
+TEST_F(BuilderTest, DISABLED_If_WithElseConditionalBreak) {
+  FAIL();
+}
+
+TEST_F(BuilderTest, If_WithContinue) {
+  ast::type::BoolType bool_type;
+  // loop {
+  //   if (true) {
+  //     continue;
+  //   }
+  // }
+  auto cond = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, true));
+
+  ast::StatementList if_body;
+  if_body.push_back(std::make_unique<ast::ContinueStatement>());
+
+  auto if_stmt =
+      std::make_unique<ast::IfStatement>(std::move(cond), std::move(if_body));
+
+  ast::StatementList loop_body;
+  loop_body.push_back(std::move(if_stmt));
+
+  ast::LoopStatement expr(std::move(loop_body), {});
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+
+  EXPECT_TRUE(b.GenerateLoopStatement(&expr)) << b.error();
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%5 = OpTypeBool
+%6 = OpConstantTrue %5
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpBranch %1
+%1 = OpLabel
+OpLoopMerge %2 %3 None
+OpBranch %4
+%4 = OpLabel
+OpSelectionMerge %7 None
+OpBranchConditional %6 %8 %7
+%8 = OpLabel
+OpBranch %3
+%7 = OpLabel
+OpBranch %3
+%3 = OpLabel
+OpBranch %1
+%2 = OpLabel
+)");
+}
+
+TEST_F(BuilderTest, DISABLED_If_WithConditionalContinue) {
+  FAIL();
+}
+TEST_F(BuilderTest, DISABLED_If_WithElseContinue) {
+  FAIL();
+}
+TEST_F(BuilderTest, DISABLED_If_WithElseConditionalContinue) {
+  FAIL();
+}
+
+TEST_F(BuilderTest, If_WithReturn) {
+  ast::type::BoolType bool_type;
+  // if (true) {
   //   return;
   // }
+  auto cond = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, true));
+
+  ast::StatementList if_body;
+  if_body.push_back(std::make_unique<ast::ReturnStatement>());
+
+  ast::IfStatement expr(std::move(cond), std::move(if_body));
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+
+  EXPECT_TRUE(b.GenerateIfStatement(&expr)) << b.error();
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool
+%2 = OpConstantTrue %1
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpSelectionMerge %3 None
+OpBranchConditional %2 %4 %3
+%4 = OpLabel
+OpReturn
+%3 = OpLabel
+)");
+}
+
+TEST_F(BuilderTest, If_WithReturnValue) {
+  ast::type::BoolType bool_type;
+  // if (true) {
+  //   return false;
+  // }
+  auto cond = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, true));
+  auto cond2 = std::make_unique<ast::ScalarConstructorExpression>(
+      std::make_unique<ast::BoolLiteral>(&bool_type, false));
+
+  ast::StatementList if_body;
+  if_body.push_back(std::make_unique<ast::ReturnStatement>(std::move(cond2)));
+
+  ast::IfStatement expr(std::move(cond), std::move(if_body));
+
+  Context ctx;
+  ast::Module mod;
+  TypeDeterminer td(&ctx, &mod);
+
+  ASSERT_TRUE(td.DetermineResultType(&expr)) << td.error();
+
+  Builder b(&mod);
+  b.push_function(Function{});
+
+  EXPECT_TRUE(b.GenerateIfStatement(&expr)) << b.error();
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool
+%2 = OpConstantTrue %1
+%5 = OpConstantFalse %1
+)");
+  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
+            R"(OpSelectionMerge %3 None
+OpBranchConditional %2 %4 %3
+%4 = OpLabel
+OpReturnValue %5
+%3 = OpLabel
+)");
 }
 
 }  // namespace