[tint][ir][val] Enforce entry point returns have location or builtin
Specifically when the return is non-void
This CL also changes some of the `CheckFunction` behaviour to not exit
early within this method for non-functional errors, i.e. if something
is null that will cause a segfault later still exit early, but if this
issue justr IR semantics like missing a decoration let the calling
code handle exiting.
This was done to make it somewhat easier to reason about what this
function was doing, since the early exit behaviour meant that the
order of checks affected the flow control in non-obvious ways.
This does introduce at least one instance of 'duplicate' errors,
i.e. a more general and specific error both being logged, but I have
filed a follow up issue (https://issues.chromium.org/issues/371219657)
about going through a proper refactoring of this function, since it is
broadly too complex as it is.
Fixes: 369813099
Change-Id: I4891db07cbad2dc0812b41ac2186ad9ba80d2648
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/209414
Auto-Submit: Ryan Harrison <rharrison@chromium.org>
Commit-Queue: James Price <jrprice@google.com>
Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/tint/lang/core/ir/validator.cc b/src/tint/lang/core/ir/validator.cc
index c9ab7c1..001778b 100644
--- a/src/tint/lang/core/ir/validator.cc
+++ b/src/tint/lang/core/ir/validator.cc
@@ -134,6 +134,12 @@
return attr.builtin.has_value() && attr.location.has_value();
}
+/// @returns true if @p attr contains one of either location or builtin decoration
+bool HasEitherLocationOrBuiltin(const tint::core::IOAttributes& attr) {
+ return (attr.builtin.has_value() && !attr.location.has_value()) ||
+ (!attr.builtin.has_value() && attr.location.has_value());
+}
+
/// @return true if @param attr does not have invariant decoration or if it also has position
/// decoration
bool InvariantOnlyIfAlsoPosition(const tint::core::IOAttributes& attr) {
@@ -1210,15 +1216,9 @@
})) {
AddError(param) << "function parameter type must be constructible, a pointer, a "
"texture, or a sampler";
- return;
}
}
- if (HasLocationAndBuiltin(param->Attributes())) {
- AddError(param) << "a builtin and location cannot be both declared for a param";
- return;
- }
-
if (auto* s = param->Type()->As<core::type::Struct>()) {
for (auto* mem : s->Members()) {
if (!InvariantOnlyIfAlsoPosition(mem->Attributes())) {
@@ -1229,29 +1229,17 @@
if (HasLocationAndBuiltin(mem->Attributes())) {
AddError(param)
<< "a builtin and location cannot be both declared for a struct member";
- return;
}
}
+ } else {
+ if (HasLocationAndBuiltin(param->Attributes())) {
+ AddError(param) << "a builtin and location cannot be both declared for a param";
+ }
}
scope_stack_.Add(param);
}
- if (HasLocationAndBuiltin(func->ReturnAttributes())) {
- AddError(func) << "a builtin and location cannot be both declared for a function return";
- return;
- }
-
- if (auto* s = func->ReturnType()->As<core::type::Struct>()) {
- for (auto* mem : s->Members()) {
- if (HasLocationAndBuiltin(mem->Attributes())) {
- AddError(func)
- << "a builtin and location cannot be both declared for a struct member";
- return;
- }
- }
- }
-
if (func->Stage() == Function::PipelineStage::kCompute) {
if (DAWN_UNLIKELY(!func->WorkgroupSize().has_value())) {
AddError(func) << "compute entry point requires workgroup size attribute";
@@ -1260,6 +1248,27 @@
if (DAWN_UNLIKELY(func->ReturnType() && !func->ReturnType()->Is<core::type::Void>())) {
AddError(func) << "compute entry point must not have a return type";
}
+ } else if (auto* s = func->ReturnType()->As<core::type::Struct>()) {
+ for (auto* mem : s->Members()) {
+ if (HasLocationAndBuiltin(mem->Attributes())) {
+ AddError(func)
+ << "a builtin and location cannot be both declared for a struct member";
+ } else if (func->Stage() != Function::PipelineStage::kUndefined &&
+ !HasEitherLocationOrBuiltin(mem->Attributes())) {
+ AddError(func) << "members of struct used for returns of entry points must "
+ "have a builtin or location decoration";
+ }
+ }
+ } else {
+ if (HasLocationAndBuiltin(func->ReturnAttributes())) {
+ AddError(func)
+ << "a builtin and location cannot be both declared for a function return";
+ } else if (!func->ReturnType()->Is<core::type::Void>() &&
+ func->Stage() != Function::PipelineStage::kUndefined &&
+ !HasEitherLocationOrBuiltin(func->ReturnAttributes())) {
+ AddError(func) << "a non-void return for an entry point must have a builtin or "
+ "location decoration";
+ }
}
// References not allowed on function signatures even with Capability::kAllowRefTypes.
@@ -1304,7 +1313,6 @@
if (func->Stage() == Function::PipelineStage::kVertex) {
CheckVertexEntryPoint(func);
- } else {
}
QueueBlock(func->Block());
diff --git a/src/tint/lang/core/ir/validator_test.cc b/src/tint/lang/core/ir/validator_test.cc
index 80bc71c..dccb69f 100644
--- a/src/tint/lang/core/ir/validator_test.cc
+++ b/src/tint/lang/core/ir/validator_test.cc
@@ -552,7 +552,7 @@
attr.location = 0;
f->SetReturnAttributes(attr);
- b.Append(f->Block(), [&] { b.Return(f); });
+ b.Append(f->Block(), [&] { b.Unreachable(); });
auto res = ir::Validate(mod);
ASSERT_NE(res, Success);
@@ -564,7 +564,7 @@
note: # Disassembly
%my_func = func():f32 [@location(0), @position] {
$B1: {
- ret
+ unreachable
}
}
)");
@@ -582,7 +582,7 @@
auto* f = b.Function("my_func", str_ty);
- b.Append(f->Block(), [&] { b.Return(f); });
+ b.Append(f->Block(), [&] { b.Unreachable(); });
auto res = ir::Validate(mod);
ASSERT_NE(res, Success);
@@ -598,7 +598,61 @@
%my_func = func():MyStruct {
$B1: {
- ret
+ unreachable
+ }
+}
+)");
+}
+
+TEST_F(IR_ValidatorTest, Function_Return_NonVoid_MissingLocationAndBuiltin) {
+ auto* f = b.Function("my_func", ty.f32());
+ f->SetStage(Function::PipelineStage::kFragment);
+
+ b.Append(f->Block(), [&] { b.Unreachable(); });
+
+ auto res = ir::Validate(mod);
+ ASSERT_NE(res, Success);
+ EXPECT_EQ(
+ res.Failure().reason.Str(),
+ R"(:1:1 error: a non-void return for an entry point must have a builtin or location decoration
+%my_func = @fragment func():f32 {
+^^^^^^^^
+
+note: # Disassembly
+%my_func = @fragment func():f32 {
+ $B1: {
+ unreachable
+ }
+}
+)");
+}
+
+TEST_F(IR_ValidatorTest, Function_Return_NonVoid_Struct_MissingLocationAndBuiltin) {
+ auto* str_ty = ty.Struct(mod.symbols.New("MyStruct"), {
+ {mod.symbols.New("a"), ty.f32(), {}},
+ });
+
+ auto* f = b.Function("my_func", str_ty);
+ f->SetStage(Function::PipelineStage::kFragment);
+
+ b.Append(f->Block(), [&] { b.Unreachable(); });
+
+ auto res = ir::Validate(mod);
+ ASSERT_NE(res, Success);
+ EXPECT_EQ(
+ res.Failure().reason.Str(),
+ R"(:5:1 error: members of struct used for returns of entry points must have a builtin or location decoration
+%my_func = @fragment func():MyStruct {
+^^^^^^^^
+
+note: # Disassembly
+MyStruct = struct @align(4) {
+ a:f32 @offset(0)
+}
+
+%my_func = @fragment func():MyStruct {
+ $B1: {
+ unreachable
}
}
)");
@@ -934,6 +988,8 @@
)");
}
+// TODO(371219657): Make only the more specific error, 'position must be declared for vertex entry
+// point output', be logged here
TEST_F(IR_ValidatorTest, Function_VertexMissingPosition) {
auto* f = b.Function("my_func", ty.vec4<f32>());
f->SetStage(Function::PipelineStage::kVertex);
@@ -941,8 +997,13 @@
auto res = ir::Validate(mod);
ASSERT_NE(res, Success);
- EXPECT_EQ(res.Failure().reason.Str(),
- R"(:1:1 error: position must be declared for vertex entry point output
+ EXPECT_EQ(
+ res.Failure().reason.Str(),
+ R"(:1:1 error: a non-void return for an entry point must have a builtin or location decoration
+%my_func = @vertex func():vec4<f32> {
+^^^^^^^^
+
+:1:1 error: position must be declared for vertex entry point output
%my_func = @vertex func():vec4<f32> {
^^^^^^^^
diff --git a/src/tint/lang/hlsl/writer/raise/shader_io_test.cc b/src/tint/lang/hlsl/writer/raise/shader_io_test.cc
index a531b62..b75a3c1 100644
--- a/src/tint/lang/hlsl/writer/raise/shader_io_test.cc
+++ b/src/tint/lang/hlsl/writer/raise/shader_io_test.cc
@@ -1076,42 +1076,39 @@
auto* subgroup_size = b.FunctionParam("size", ty.u32());
subgroup_size->SetBuiltin(core::BuiltinValue::kSubgroupSize);
- auto* ep = b.Function("foo", ty.u32(), core::ir::Function::PipelineStage::kFragment);
+ auto* ep = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kFragment);
ep->SetParams({subgroup_invocation_id, subgroup_size});
b.Append(ep->Block(), [&] {
- auto* r = b.Multiply(ty.u32(), subgroup_invocation_id, subgroup_size);
- b.Return(ep, r);
+ b.Let("x", b.Multiply(ty.u32(), subgroup_invocation_id, subgroup_size));
+ b.Return(ep);
});
auto* src = R"(
-%foo = @fragment func(%id:u32 [@subgroup_invocation_id], %size:u32 [@subgroup_size]):u32 {
+%foo = @fragment func(%id:u32 [@subgroup_invocation_id], %size:u32 [@subgroup_size]):void {
$B1: {
%4:u32 = mul %id, %size
- ret %4
+ %x:u32 = let %4
+ ret
}
}
)";
EXPECT_EQ(src, str());
auto* expect = R"(
-foo_outputs = struct @align(4) {
- tint_symbol:u32 @offset(0)
-}
-
-%foo_inner = func(%id:u32, %size:u32):u32 {
+%foo_inner = func(%id:u32, %size:u32):void {
$B1: {
%4:u32 = mul %id, %size
- ret %4
+ %x:u32 = let %4
+ ret
}
}
-%foo = @fragment func():foo_outputs {
+%foo = @fragment func():void {
$B2: {
- %6:u32 = hlsl.WaveGetLaneIndex
- %7:u32 = hlsl.WaveGetLaneCount
- %8:u32 = call %foo_inner, %6, %7
- %9:foo_outputs = construct %8
- ret %9
+ %7:u32 = hlsl.WaveGetLaneIndex
+ %8:u32 = hlsl.WaveGetLaneCount
+ %9:void = call %foo_inner, %7, %8
+ ret
}
}
)";
@@ -1152,14 +1149,14 @@
auto* str_param = b.FunctionParam("inputs", str_ty);
- auto* ep = b.Function("foo", ty.u32(), core::ir::Function::PipelineStage::kFragment);
+ auto* ep = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kFragment);
ep->SetParams({str_param});
b.Append(ep->Block(), [&] {
auto* subgroup_invocation_id = b.Access(ty.u32(), str_param, 0_i);
auto* subgroup_size = b.Access(ty.u32(), str_param, 1_i);
- auto* r = b.Multiply(ty.u32(), subgroup_invocation_id, subgroup_size);
- b.Return(ep, r);
+ b.Let("x", b.Multiply(ty.u32(), subgroup_invocation_id, subgroup_size));
+ b.Return(ep);
});
auto* src = R"(
@@ -1168,12 +1165,13 @@
size:u32 @offset(4), @builtin(subgroup_size)
}
-%foo = @fragment func(%inputs:Inputs):u32 {
+%foo = @fragment func(%inputs:Inputs):void {
$B1: {
%3:u32 = access %inputs, 0i
%4:u32 = access %inputs, 1i
%5:u32 = mul %3, %4
- ret %5
+ %x:u32 = let %5
+ ret
}
}
)";
@@ -1185,26 +1183,22 @@
size:u32 @offset(4)
}
-foo_outputs = struct @align(4) {
- tint_symbol:u32 @offset(0)
-}
-
-%foo_inner = func(%inputs:Inputs):u32 {
+%foo_inner = func(%inputs:Inputs):void {
$B1: {
%3:u32 = access %inputs, 0i
%4:u32 = access %inputs, 1i
%5:u32 = mul %3, %4
- ret %5
+ %x:u32 = let %5
+ ret
}
}
-%foo = @fragment func():foo_outputs {
+%foo = @fragment func():void {
$B2: {
- %7:u32 = hlsl.WaveGetLaneIndex
- %8:u32 = hlsl.WaveGetLaneCount
- %9:Inputs = construct %7, %8
- %10:u32 = call %foo_inner, %9
- %11:foo_outputs = construct %10
- ret %11
+ %8:u32 = hlsl.WaveGetLaneIndex
+ %9:u32 = hlsl.WaveGetLaneCount
+ %10:Inputs = construct %8, %9
+ %11:void = call %foo_inner, %10
+ ret
}
}
)";
