writer/spirv: Fix trailing emission of OpReturn

writer::spirv::Function attempted to append a trailing OpReturn if the
function does not end with a terminating instruction. This wasn't
considering functions that have a non-void return type.

This has now been moved to spirv::Builder::GenerateFunction(),
where we can actually examine the function return type, and generate a
zero-expression to return if we need to.

Note: this was masked by WGSL validation that required all functions to
end with a return statement.

Bug: tint:1302
Change-Id: Iddfeda25a956622c318b8235dc6fc093a2a5c26d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/71604
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>

src/writer/spirv/builder.cc

@@ -653,6 +653,15 @@ bool Builder::GenerateFunction(const ast::Function* func_ast) {
     }
   }
 
+  if (!LastIsTerminator(func_ast->body)) {
+    if (func->ReturnType()->Is<sem::Void>()) {
+      push_function_inst(spv::Op::OpReturn, {});
+    } else {
+      auto zero = GenerateConstantNullIfNeeded(func->ReturnType());
+      push_function_inst(spv::Op::OpReturnValue, {Operand::Int(zero)});
+    }
+  }
+
   if (func_ast->IsEntryPoint()) {
     if (!GenerateEntryPoint(func_ast, func_id)) {
       return false;

src/writer/spirv/builder_accessor_expression_test.cc

@@ -787,6 +787,7 @@ TEST_F(BuilderTest, IndexAccessor_Of_Vec) {
   EXPECT_EQ(DumpInstructions(b.functions()[0].variables()), R"()");
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
             R"(%18 = OpCompositeExtract %6 %16 1
+OpReturn
 )");
 
   Validate(b);
@@ -832,6 +833,7 @@ TEST_F(BuilderTest, IndexAccessor_Of_Array_Of_f32) {
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
             R"(%18 = OpCompositeExtract %6 %16 2
 %20 = OpCompositeExtract %7 %18 1
+OpReturn
 )");
 
   Validate(b);
@@ -931,6 +933,7 @@ TEST_F(BuilderTest, IndexAccessor_Array_Literal) {
   EXPECT_EQ(DumpInstructions(b.functions()[0].variables()), "");
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
             R"(%15 = OpCompositeExtract %6 %12 2
+OpReturn
 )");
 
   Validate(b);
@@ -979,6 +982,7 @@ TEST_F(BuilderTest, IndexAccessor_Array_Dynamic) {
 %20 = OpLoad %18 %16
 %22 = OpAccessChain %21 %13 %20
 %23 = OpLoad %6 %22
+OpReturn
 )");
 
   Validate(b);
@@ -1031,6 +1035,7 @@ TEST_F(BuilderTest, IndexAccessor_Matrix_Dynamic) {
 %22 = OpLoad %20 %18
 %24 = OpAccessChain %23 %15 %22
 %25 = OpLoad %6 %24
+OpReturn
 )");
 
   Validate(b);

src/writer/spirv/builder_call_test.cc

@@ -29,23 +29,15 @@ TEST_F(BuilderTest, Expression_Call) {
   func_params.push_back(Param("a", ty.f32()));
   func_params.push_back(Param("b", ty.f32()));
 
-  auto* a_func =
+  auto* a_func = Func("a_func", func_params, ty.f32(), {Return(Add("a", "b"))});
-      Func("a_func", func_params, ty.f32(),
-           ast::StatementList{Return(Add("a", "b"))}, ast::DecorationList{});
-
   auto* func =
-      Func("main", {}, ty.void_(), ast::StatementList{}, ast::DecorationList{});
+      Func("main", {}, ty.void_(), {Assign(Phony(), Call("a_func", 1.f, 1.f))});
-
-  auto* expr = Call("a_func", 1.f, 1.f);
-
-  WrapInFunction(expr);
 
   spirv::Builder& b = Build();
 
   ASSERT_TRUE(b.GenerateFunction(a_func)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_EQ(b.GenerateCallExpression(expr), 12u) << b.error();
   EXPECT_EQ(DumpBuilder(b), R"(OpName %3 "a_func"
 OpName %4 "a"
 OpName %5 "b"
@@ -75,23 +67,16 @@ TEST_F(BuilderTest, Statement_Call) {
   func_params.push_back(Param("a", ty.f32()));
   func_params.push_back(Param("b", ty.f32()));
 
-  auto* a_func =
+  auto* a_func = Func("a_func", func_params, ty.f32(), {Return(Add("a", "b"))});
-      Func("a_func", func_params, ty.f32(),
-           ast::StatementList{Return(Add("a", "b"))}, ast::DecorationList{});
 
   auto* func =
-      Func("main", {}, ty.void_(), ast::StatementList{}, ast::DecorationList{});
+      Func("main", {}, ty.void_(), {CallStmt(Call("a_func", 1.f, 1.f))});
-
-  auto* expr = CallStmt(Call("a_func", 1.f, 1.f));
-
-  WrapInFunction(expr);
 
   spirv::Builder& b = Build();
 
   ASSERT_TRUE(b.GenerateFunction(a_func)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateStatement(expr)) << b.error();
   EXPECT_EQ(DumpBuilder(b), R"(OpName %3 "a_func"
 OpName %4 "a"
 OpName %5 "b"

src/writer/spirv/builder_if_test.cc

@@ -452,26 +452,24 @@ TEST_F(BuilderTest, If_WithReturn) {
   // if (true) {
   //   return;
   // }
-  auto* if_body = Block(Return());
 
-  auto* expr =
+  auto* fn = Func("f", {}, ty.void_(), {If(true, Block(Return()))});
-      create<ast::IfStatement>(Expr(true), if_body, ast::ElseStatementList{});
-  WrapInFunction(expr);
 
   spirv::Builder& b = Build();
 
-  b.push_function(Function{});
+  EXPECT_TRUE(b.GenerateFunction(fn)) << b.error();
-
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeVoid
-  EXPECT_TRUE(b.GenerateIfStatement(expr)) << b.error();
+%1 = OpTypeFunction %2
-  EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool
+%5 = OpTypeBool
-%2 = OpConstantTrue %1
+%6 = OpConstantTrue %5
 )");
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
-            R"(OpSelectionMerge %3 None
+            R"(OpSelectionMerge %7 None
-OpBranchConditional %2 %4 %3
+OpBranchConditional %6 %8 %7
-%4 = OpLabel
+%8 = OpLabel
+OpReturn
+%7 = OpLabel
 OpReturn
-%3 = OpLabel
 )");
 }
 
@@ -480,24 +478,28 @@ TEST_F(BuilderTest, If_WithReturnValue) {
   //   return false;
   // }
   // return true;
-  auto* if_body = Block(Return(false));
+
-  auto* expr = If(Expr(true), if_body);
+  auto* fn = Func("f", {}, ty.bool_(),
-  Func("test", {}, ty.bool_(), {expr, Return(true)}, {});
+                  {
+                      If(true, Block(Return(false))),
+                      Return(true),
+                  });
+
   spirv::Builder& b = Build();
 
-  b.push_function(Function{});
+  EXPECT_TRUE(b.GenerateFunction(fn)) << b.error();
-
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeBool
-  EXPECT_TRUE(b.GenerateIfStatement(expr)) << b.error();
+%1 = OpTypeFunction %2
-  EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool
+%5 = OpConstantTrue %2
-%2 = OpConstantTrue %1
+%8 = OpConstantFalse %2
-%5 = OpConstantFalse %1
 )");
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
-            R"(OpSelectionMerge %3 None
+            R"(OpSelectionMerge %6 None
-OpBranchConditional %2 %4 %3
+OpBranchConditional %5 %7 %6
-%4 = OpLabel
+%7 = OpLabel
+OpReturnValue %8
+%6 = OpLabel
 OpReturnValue %5
-%3 = OpLabel
 )");
 }
 
@@ -512,24 +514,28 @@ TEST_F(BuilderTest, If_WithNestedBlockReturnValue) {
   //  }
   // }
   // return true;
-  auto* if_body = Block(Block(Block(Block(Return(false)))));
+
-  auto* expr = If(Expr(true), if_body);
+  auto* fn = Func("f", {}, ty.bool_(),
-  Func("test", {}, ty.bool_(), {expr, Return(true)}, {});
+                  {
+                      If(true, Block(Block(Block(Block(Return(false)))))),
+                      Return(true),
+                  });
+
   spirv::Builder& b = Build();
 
-  b.push_function(Function{});
+  EXPECT_TRUE(b.GenerateFunction(fn)) << b.error();
-
+  EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeBool
-  EXPECT_TRUE(b.GenerateIfStatement(expr)) << b.error();
+%1 = OpTypeFunction %2
-  EXPECT_EQ(DumpInstructions(b.types()), R"(%1 = OpTypeBool
+%5 = OpConstantTrue %2
-%2 = OpConstantTrue %1
+%8 = OpConstantFalse %2
-%5 = OpConstantFalse %1
 )");
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
-            R"(OpSelectionMerge %3 None
+            R"(OpSelectionMerge %6 None
-OpBranchConditional %2 %4 %3
+OpBranchConditional %5 %7 %6
-%4 = OpLabel
+%7 = OpLabel
+OpReturnValue %8
+%6 = OpLabel
 OpReturnValue %5
-%3 = OpLabel
 )");
 }
 
@@ -539,29 +545,27 @@ TEST_F(BuilderTest, If_WithLoad_Bug327) {
   // }
 
   auto* var = Global("a", ty.bool_(), ast::StorageClass::kPrivate);
-
+  auto* fn = Func("f", {}, ty.void_(), {If("a", Block())});
-  auto* expr =
-      create<ast::IfStatement>(Expr("a"), Block(), ast::ElseStatementList{});
-  WrapInFunction(expr);
 
   spirv::Builder& b = Build();
 
-  b.push_function(Function{});
   ASSERT_TRUE(b.GenerateGlobalVariable(var)) << b.error();
-
+  EXPECT_TRUE(b.GenerateFunction(fn)) << b.error();
-  EXPECT_TRUE(b.GenerateIfStatement(expr)) << b.error();
   EXPECT_EQ(DumpInstructions(b.types()), R"(%3 = OpTypeBool
 %2 = OpTypePointer Private %3
 %4 = OpConstantNull %3
 %1 = OpVariable %2 Private %4
+%6 = OpTypeVoid
+%5 = OpTypeFunction %6
 )");
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
-            R"(%5 = OpLoad %3 %1
+            R"(%9 = OpLoad %3 %1
-OpSelectionMerge %6 None
+OpSelectionMerge %10 None
-OpBranchConditional %5 %7 %6
+OpBranchConditional %9 %11 %10
-%7 = OpLabel
+%11 = OpLabel
-OpBranch %6
+OpBranch %10
-%6 = OpLabel
+%10 = OpLabel
+OpReturn
 )");
 }
 

src/writer/spirv/builder_switch_test.cc

@@ -60,13 +60,13 @@ TEST_F(BuilderTest, Switch_WithCase) {
   auto* v = Global("v", ty.i32(), ast::StorageClass::kPrivate);
   auto* a = Global("a", ty.i32(), ast::StorageClass::kPrivate);
 
-  auto* expr = Switch("a", /**/
+  auto* func = Func("a_func", {}, ty.void_(),
-                      Case(Expr(1), Block(Assign("v", 1))),
+                    {
-                      Case(Expr(2), Block(Assign("v", 2))), DefaultCase());
+                        Switch("a",                                   //
-  WrapInFunction(expr);
+                               Case(Expr(1), Block(Assign("v", 1))),  //
-
+                               Case(Expr(2), Block(Assign("v", 2))),  //
-  auto* func = Func("a_func", {}, ty.void_(), ast::StatementList{},
+                               DefaultCase()),
-                    ast::DecorationList{});
+                    });
 
   spirv::Builder& b = Build();
 
@@ -74,8 +74,6 @@ TEST_F(BuilderTest, Switch_WithCase) {
   ASSERT_TRUE(b.GenerateGlobalVariable(a)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateSwitchStatement(expr)) << b.error();
-
   EXPECT_EQ(DumpBuilder(b), R"(OpName %1 "v"
 OpName %5 "a"
 OpName %8 "a_func"
@@ -119,13 +117,13 @@ TEST_F(BuilderTest, Switch_WithCase_Unsigned) {
   auto* v = Global("v", ty.i32(), ast::StorageClass::kPrivate);
   auto* a = Global("a", ty.u32(), ast::StorageClass::kPrivate);
 
-  auto* expr = Switch("a", Case(Expr(1u), Block(Assign("v", 1))),
+  auto* func = Func("a_func", {}, ty.void_(),
-                      Case(Expr(2u), Block(Assign("v", 2))), DefaultCase());
+                    {
-
+                        Switch("a",                                    //
-  WrapInFunction(expr);
+                               Case(Expr(1u), Block(Assign("v", 1))),  //
-
+                               Case(Expr(2u), Block(Assign("v", 2))),  //
-  auto* func = Func("a_func", {}, ty.void_(), ast::StatementList{},
+                               DefaultCase()),
-                    ast::DecorationList{});
+                    });
 
   spirv::Builder& b = Build();
 
@@ -133,8 +131,6 @@ TEST_F(BuilderTest, Switch_WithCase_Unsigned) {
   ASSERT_TRUE(b.GenerateGlobalVariable(a)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateSwitchStatement(expr)) << b.error();
-
   EXPECT_EQ(DumpBuilder(b), R"(OpName %1 "v"
 OpName %5 "a"
 OpName %11 "a_func"
@@ -178,18 +174,11 @@ TEST_F(BuilderTest, Switch_WithDefault) {
   auto* v = Global("v", ty.i32(), ast::StorageClass::kPrivate);
   auto* a = Global("a", ty.i32(), ast::StorageClass::kPrivate);
 
-  auto* default_body = Block(Assign("v", 1));
+  auto* func = Func("a_func", {}, ty.void_(),
-
+                    {
-  ast::CaseStatementList cases;
+                        Switch("a",                                  //
-  cases.push_back(
+                               DefaultCase(Block(Assign("v", 1)))),  //
-      create<ast::CaseStatement>(ast::CaseSelectorList{}, default_body));
+                    });
-
-  auto* expr = create<ast::SwitchStatement>(Expr("a"), cases);
-
-  WrapInFunction(expr);
-
-  auto* func = Func("a_func", {}, ty.void_(), ast::StatementList{},
-                    ast::DecorationList{});
 
   spirv::Builder& b = Build();
 
@@ -197,8 +186,6 @@ TEST_F(BuilderTest, Switch_WithDefault) {
   ASSERT_TRUE(b.GenerateGlobalVariable(a)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateSwitchStatement(expr)) << b.error();
-
   EXPECT_EQ(DumpBuilder(b), R"(OpName %1 "v"
 OpName %5 "a"
 OpName %8 "a_func"
@@ -237,31 +224,15 @@ TEST_F(BuilderTest, Switch_WithCaseAndDefault) {
   auto* v = Global("v", ty.i32(), ast::StorageClass::kPrivate);
   auto* a = Global("a", ty.i32(), ast::StorageClass::kPrivate);
 
-  auto* case_1_body = Block(Assign("v", Expr(1)));
+  auto* func = Func("a_func", {}, ty.void_(),
-
+                    {
-  auto* case_2_body = Block(Assign("v", Expr(2)));
+                        Switch(Expr("a"),                    //
-
+                               Case(Expr(1),                 //
-  auto* default_body = Block(Assign("v", Expr(3)));
+                                    Block(Assign("v", 1))),  //
-
+                               Case({Expr(2), Expr(3)},      //
-  ast::CaseSelectorList selector_1;
+                                    Block(Assign("v", 2))),  //
-  selector_1.push_back(Expr(1));
+                               DefaultCase(Block(Assign("v", 3)))),
-
+                    });
-  ast::CaseSelectorList selector_2;
-  selector_2.push_back(Expr(2));
-  selector_2.push_back(Expr(3));
-
-  ast::CaseStatementList cases;
-  cases.push_back(create<ast::CaseStatement>(selector_1, case_1_body));
-  cases.push_back(create<ast::CaseStatement>(selector_2, case_2_body));
-  cases.push_back(
-      create<ast::CaseStatement>(ast::CaseSelectorList{}, default_body));
-
-  auto* expr = create<ast::SwitchStatement>(Expr("a"), cases);
-
-  WrapInFunction(expr);
-
-  auto* func = Func("a_func", {}, ty.void_(), ast::StatementList{},
-                    ast::DecorationList{});
 
   spirv::Builder& b = Build();
 
@@ -269,8 +240,6 @@ TEST_F(BuilderTest, Switch_WithCaseAndDefault) {
   ASSERT_TRUE(b.GenerateGlobalVariable(a)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateSwitchStatement(expr)) << b.error();
-
   EXPECT_EQ(DumpBuilder(b), R"(OpName %1 "v"
 OpName %5 "a"
 OpName %8 "a_func"
@@ -318,31 +287,15 @@ TEST_F(BuilderTest, Switch_CaseWithFallthrough) {
   auto* v = Global("v", ty.i32(), ast::StorageClass::kPrivate);
   auto* a = Global("a", ty.i32(), ast::StorageClass::kPrivate);
 
-  auto* case_1_body =
+  auto* func = Func("a_func", {}, ty.void_(),
-      Block(Assign("v", Expr(1)), create<ast::FallthroughStatement>());
+                    {
-
+                        Switch(Expr("a"),                                   //
-  auto* case_2_body = Block(Assign("v", Expr(2)));
+                               Case(Expr(1),                                //
-
+                                    Block(Assign("v", 1), Fallthrough())),  //
-  auto* default_body = Block(Assign("v", Expr(3)));
+                               Case(Expr(2),                                //
-
+                                    Block(Assign("v", 2))),                 //
-  ast::CaseSelectorList selector_1;
+                               DefaultCase(Block(Assign("v", 3)))),
-  selector_1.push_back(Expr(1));
+                    });
-
-  ast::CaseSelectorList selector_2;
-  selector_2.push_back(Expr(2));
-
-  ast::CaseStatementList cases;
-  cases.push_back(create<ast::CaseStatement>(selector_1, case_1_body));
-  cases.push_back(create<ast::CaseStatement>(selector_2, case_2_body));
-  cases.push_back(
-      create<ast::CaseStatement>(ast::CaseSelectorList{}, default_body));
-
-  auto* expr = create<ast::SwitchStatement>(Expr("a"), cases);
-
-  WrapInFunction(expr);
-
-  auto* func = Func("a_func", {}, ty.void_(), ast::StatementList{},
-                    ast::DecorationList{});
 
   spirv::Builder& b = Build();
 
@@ -350,8 +303,6 @@ TEST_F(BuilderTest, Switch_CaseWithFallthrough) {
   ASSERT_TRUE(b.GenerateGlobalVariable(a)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateSwitchStatement(expr)) << b.error();
-
   EXPECT_EQ(DumpBuilder(b), R"(OpName %1 "v"
 OpName %5 "a"
 OpName %8 "a_func"
@@ -398,17 +349,16 @@ TEST_F(BuilderTest, Switch_WithNestedBreak) {
   auto* v = Global("v", ty.i32(), ast::StorageClass::kPrivate);
   auto* a = Global("a", ty.i32(), ast::StorageClass::kPrivate);
 
-  auto* expr = Switch(
+  auto* func = Func(
-      "a",                /**/
+      "a_func", {}, ty.void_(),
-      Case(Expr(1), Block(/**/
+      {
+          Switch("a",           //
+                 Case(Expr(1),  //
+                      Block(    //
                           If(Expr(true), Block(create<ast::BreakStatement>())),
                           Assign("v", 1))),
-      DefaultCase());
+                 DefaultCase()),
-
+      });
-  WrapInFunction(expr);
-
-  auto* func = Func("a_func", {}, ty.void_(), ast::StatementList{},
-                    ast::DecorationList{});
 
   spirv::Builder& b = Build();
 
@@ -416,8 +366,6 @@ TEST_F(BuilderTest, Switch_WithNestedBreak) {
   ASSERT_TRUE(b.GenerateGlobalVariable(a)) << b.error();
   ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
 
-  EXPECT_TRUE(b.GenerateSwitchStatement(expr)) << b.error();
-
   EXPECT_EQ(DumpBuilder(b), R"(OpName %1 "v"
 OpName %5 "a"
 OpName %8 "a_func"

src/writer/spirv/function.cc

@@ -17,15 +17,6 @@
 namespace tint {
 namespace writer {
 namespace spirv {
-namespace {
-
-// Returns true if the given Op is a function terminator
-bool OpIsFunctionTerminator(spv::Op op) {
-  return op == spv::Op::OpReturn || op == spv::Op::OpReturnValue ||
-         op == spv::Op::OpKill;
-}
-
-}  // namespace
 
 Function::Function()
     : declaration_(Instruction{spv::Op::OpNop, {}}),
@@ -56,17 +47,6 @@ void Function::iterate(std::function<void(const Instruction&)> cb) const {
     cb(inst);
   }
 
-  bool needs_terminator = false;
-  if (instructions_.empty()) {
-    needs_terminator = true;
-  } else {
-    const auto& last = instructions_.back();
-    needs_terminator = !OpIsFunctionTerminator(last.opcode());
-  }
-  if (needs_terminator) {
-    cb(Instruction{spv::Op::OpReturn, {}});
-  }
-
   cb(Instruction{spv::Op::OpFunctionEnd, {}});
 }