From 3d5453ce9c35ffa906563c9009423af3b91890bc Mon Sep 17 00:00:00 2001 From: David Neto Date: Mon, 21 Jun 2021 19:21:36 +0000 Subject: [PATCH] spirv-reader: remove builtin-inputs special cases Remove special case handling of pointers and values related to builtins SampleId, VertexIndex, and InstanceIndex. These map to private variables with store type matching the type stated in the SPIR-V code. There is no need to generate special case code for user-written functions accessing those variables. Therefore: - Remove SkipReason enums associated with those builtin inputs - Remove newly unreachable code. Bug: tint:508 Change-Id: I22ea86d49e14f171a92863d9f02145606ad37683 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/55321 Kokoro: Kokoro Reviewed-by: James Price Commit-Queue: James Price Auto-Submit: David Neto --- src/reader/spirv/function.cc | 64 +----- src/reader/spirv/function.h | 29 --- .../spirv/parser_impl_module_var_test.cc | 205 ++++++++++++------ 3 files changed, 147 insertions(+), 151 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 9cde282c7c..c1be6f18f5 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -2309,20 +2309,6 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) { Fail() << "unhandled use of a pointer to the PointSize builtin, with ID: " << id; return {}; - case SkipReason::kSampleIdBuiltinPointer: - Fail() << "unhandled use of a pointer to the SampleId builtin, with ID: " - << id; - return {}; - case SkipReason::kVertexIndexBuiltinPointer: - Fail() - << "unhandled use of a pointer to the VertexIndex builtin, with ID: " - << id; - return {}; - case SkipReason::kInstanceIndexBuiltinPointer: - Fail() << "unhandled use of a pointer to the InstanceIndex builtin, with " - "ID: " - << id; - return {}; case SkipReason::kSampleMaskInBuiltinPointer: Fail() << "unhandled use of a pointer to the SampleMask builtin, with ID: " @@ -3423,22 +3409,6 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { GetDefInfo(inst.result_id())->skip = SkipReason::kPointSizeBuiltinValue; return true; - case SkipReason::kSampleIdBuiltinPointer: - case SkipReason::kVertexIndexBuiltinPointer: - case SkipReason::kInstanceIndexBuiltinPointer: { - auto name = NameForSpecialInputBuiltin(skip_reason); - if (name.empty()) { - return Fail() << "internal error: unhandled special input builtin " - "variable: " - << inst.PrettyPrint(); - } - ast::Expression* id_expr = create( - Source{}, builder_.Symbols().Register(name)); - - auto* loaded_type = parser_impl_.ConvertType(inst.type_id()); - auto expr = TypedExpression{loaded_type, id_expr}; - return EmitConstDefinition(inst, expr); - } case SkipReason::kSampleMaskInBuiltinPointer: { auto name = namer_.Name(sample_mask_in_id); ast::Expression* id_expr = create( @@ -3573,28 +3543,6 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { << inst.PrettyPrint(); } -std::string FunctionEmitter::NameForSpecialInputBuiltin( - SkipReason skip_reason) { - SpvBuiltIn spv_builtin = SpvBuiltIn(0); - switch (skip_reason) { - case SkipReason::kSampleIdBuiltinPointer: - spv_builtin = SpvBuiltInSampleId; - break; - case SkipReason::kVertexIndexBuiltinPointer: - spv_builtin = SpvBuiltInVertexIndex; - break; - case SkipReason::kInstanceIndexBuiltinPointer: - spv_builtin = SpvBuiltInInstanceIndex; - break; - default: - // Invalid. Issue the error in the caller. - return ""; - } - // The SPIR-V variable is i32, but WGSL requires u32. - auto var_id = parser_impl_.IdForSpecialBuiltIn(spv_builtin); - return namer_.Name(var_id); -} - TypedExpression FunctionEmitter::MakeOperand( const spvtools::opt::Instruction& inst, uint32_t operand_index) { @@ -4279,15 +4227,6 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() { case SpvBuiltInPointSize: def->skip = SkipReason::kPointSizeBuiltinPointer; break; - case SpvBuiltInSampleId: - def->skip = SkipReason::kSampleIdBuiltinPointer; - break; - case SpvBuiltInVertexIndex: - def->skip = SkipReason::kVertexIndexBuiltinPointer; - break; - case SpvBuiltInInstanceIndex: - def->skip = SkipReason::kInstanceIndexBuiltinPointer; - break; case SpvBuiltInSampleMask: { // Distinguish between input and output variable. const auto storage_class = @@ -4301,6 +4240,9 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() { } break; } + case SpvBuiltInSampleId: + case SpvBuiltInInstanceIndex: + case SpvBuiltInVertexIndex: case SpvBuiltInLocalInvocationIndex: case SpvBuiltInLocalInvocationId: case SpvBuiltInGlobalInvocationId: diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index 944afd54a3..72a357f5a2 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -214,19 +214,6 @@ enum class SkipReason { /// supported by WebGPU. kPointSizeBuiltinValue, - /// `kSampleIdBuiltinPointer`: the value is a pointer to the SampleId builtin - /// variable. Don't generate its address. - kSampleIdBuiltinPointer, - - /// `kVertexIndexBuiltinPointer`: the value is a pointer to the VertexIndex - /// builtin variable. Don't generate its address. - kVertexIndexBuiltinPointer, - - /// `kInstanceIndexBuiltinPointer`: the value is a pointer to the - /// InstanceIndex - /// builtin variable. Don't generate its address. - kInstanceIndexBuiltinPointer, - /// `kSampleMaskInBuiltinPointer`: the value is a pointer to the SampleMaskIn /// builtin input variable. Don't generate its address. kSampleMaskInBuiltinPointer, @@ -339,15 +326,6 @@ inline std::ostream& operator<<(std::ostream& o, const DefInfo& di) { case SkipReason::kPointSizeBuiltinValue: o << " skip:pointsize_value"; break; - case SkipReason::kSampleIdBuiltinPointer: - o << " skip:sampleid_pointer"; - break; - case SkipReason::kVertexIndexBuiltinPointer: - o << " skip:vertexindex_pointer"; - break; - case SkipReason::kInstanceIndexBuiltinPointer: - o << " skip:instanceindex_pointer"; - break; case SkipReason::kSampleMaskInBuiltinPointer: o << " skip:samplemaskin_pointer"; break; @@ -795,13 +773,6 @@ class FunctionEmitter { return SkipReason::kDontSkip; } - /// Returns the WGSL variable name for an input builtin variable whose - /// translation is managed via the SkipReason mechanism. - /// @param skip_reason the skip reason for the special variable - /// @returns the variable name for a special builtin variable - /// that is handled via the "skip" mechanism. - std::string NameForSpecialInputBuiltin(SkipReason skip_reason); - /// Returns the most deeply nested structured construct which encloses the /// WGSL scopes of names declared in both block positions. Each position must /// be a valid index into the function block order array. diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc index c4f3679d2e..c32fb8f105 100644 --- a/src/reader/spirv/parser_impl_module_var_test.cc +++ b/src/reader/spirv/parser_impl_module_var_test.cc @@ -2443,18 +2443,31 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_CopyObject) { ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly; EXPECT_TRUE(p->error().empty()); const auto module_str = p->program().to_str(); - - // Correct declaration - EXPECT_THAT(module_str, HasSubstr(R"( + const std::string expected = + R"(Module{ Variable{ x_1 private undefined __i32 - })")) < __void + () + { + VariableDeclStatement{ + VariableConst{ + x_11 + none + undefined + __ptr_none__i32 + { + UnaryOp[not set]{ + address-of + Identifier[not set]{x_1} + } + } + } + } VariableDeclStatement{ VariableConst{ x_2 @@ -2462,14 +2475,15 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_CopyObject) { undefined __i32 { - Identifier[not set]{x_1} + UnaryOp[not set]{ + indirection + Identifier[not set]{x_11} + } } } - })")) - << module_str; - - // Correct parameter on entry point - EXPECT_THAT(module_str, HasSubstr(R"( + } + Return{} + } Function main -> __void StageDecoration{fragment} ( @@ -2489,8 +2503,15 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_CopyObject) { Bitcast[not set]<__i32>{ Identifier[not set]{x_1_param} } - })")) - << module_str; + } + Call[not set]{ + Identifier[not set]{main_1} + ( + ) + } + } +} +)"; } TEST_F(SpvModuleScopeVarParserTest, SampleId_I32_Load_AccessChain) { @@ -2668,6 +2689,20 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_U32_Load_CopyObject) { Function main_1 -> __void () { + VariableDeclStatement{ + VariableConst{ + x_11 + none + undefined + __ptr_none__u32 + { + UnaryOp[not set]{ + address-of + Identifier[not set]{x_1} + } + } + } + } VariableDeclStatement{ VariableConst{ x_2 @@ -2675,7 +2710,10 @@ TEST_F(SpvModuleScopeVarParserTest, SampleId_U32_Load_CopyObject) { undefined __u32 { - Identifier[not set]{x_1} + UnaryOp[not set]{ + indirection + Identifier[not set]{x_11} + } } } } @@ -2944,18 +2982,16 @@ TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_CopyObject) { ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly; EXPECT_TRUE(p->error().empty()); const auto module_str = p->program().to_str(); - - // Correct declaration - EXPECT_THAT(module_str, HasSubstr(R"( + const std::string expected = R"(Module{ Variable{ x_1 private undefined __array__u32_1 - })")) < __void + () + { VariableDeclStatement{ VariableConst{ x_4 @@ -2969,10 +3005,9 @@ TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_CopyObject) { } } } - })")); - - // Correct parameter on entry point - EXPECT_THAT(module_str, HasSubstr(R"( + } + Return{} + } Function main -> __void StageDecoration{fragment} ( @@ -2993,8 +3028,16 @@ TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_CopyObject) { ScalarConstructor[not set]{0} } Identifier[not set]{x_1_param} - })")) - << module_str; + } + Call[not set]{ + Identifier[not set]{main_1} + ( + ) + } + } +} +)"; + EXPECT_EQ(module_str, expected) << module_str; } TEST_F(SpvModuleScopeVarParserTest, SampleMask_In_U32_AccessChain) { @@ -3955,6 +3998,20 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_CopyObject) { Function main_1 -> __void () { + VariableDeclStatement{ + VariableConst{ + x_11 + none + undefined + __ptr_none__i32 + { + UnaryOp[not set]{ + address-of + Identifier[not set]{x_1} + } + } + } + } VariableDeclStatement{ VariableConst{ x_2 @@ -3962,7 +4019,10 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_CopyObject) { undefined __i32 { - Identifier[not set]{x_1} + UnaryOp[not set]{ + indirection + Identifier[not set]{x_11} + } } } } @@ -4066,34 +4126,6 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_AccessChain) { EXPECT_EQ(module_str, expected); } -TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_FunctParam) { - // TODO(dneto): Passing by pointer-to-input is not allowed. - // Remove this test. - const std::string assembly = VertexIndexPreamble("%int") + R"( - %helper_ty = OpTypeFunction %int %ptr_ty - %helper = OpFunction %int None %helper_ty - %param = OpFunctionParameter %ptr_ty - %helper_entry = OpLabel - %3 = OpLoad %int %param - OpReturnValue %3 - OpFunctionEnd - - %main = OpFunction %void None %voidfn - %entry = OpLabel - %result = OpFunctionCall %int %helper %1 - OpReturn - OpFunctionEnd - )"; - auto p = parser(test::Assemble(assembly)); - // TODO(dneto): We can handle this if we make a shadow variable and mutate - // the parameter type. - ASSERT_FALSE(p->BuildAndParseInternalModule()); - EXPECT_THAT( - p->error(), - HasSubstr( - "unhandled use of a pointer to the VertexIndex builtin, with ID: 1")); -} - TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_Load_Direct) { const std::string assembly = VertexIndexPreamble("%uint") + R"( %main = OpFunction %void None %voidfn @@ -4181,6 +4213,20 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_Load_CopyObject) { Function main_1 -> __void () { + VariableDeclStatement{ + VariableConst{ + x_11 + none + undefined + __ptr_none__u32 + { + UnaryOp[not set]{ + address-of + Identifier[not set]{x_1} + } + } + } + } VariableDeclStatement{ VariableConst{ x_2 @@ -4188,7 +4234,10 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_Load_CopyObject) { undefined __u32 { - Identifier[not set]{x_1} + UnaryOp[not set]{ + indirection + Identifier[not set]{x_11} + } } } } @@ -4421,6 +4470,20 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_CopyObject) { Function main_1 -> __void () { + VariableDeclStatement{ + VariableConst{ + x_11 + none + undefined + __ptr_none__i32 + { + UnaryOp[not set]{ + address-of + Identifier[not set]{x_1} + } + } + } + } VariableDeclStatement{ VariableConst{ x_2 @@ -4428,7 +4491,10 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_CopyObject) { undefined __i32 { - Identifier[not set]{x_1} + UnaryOp[not set]{ + indirection + Identifier[not set]{x_11} + } } } } @@ -4643,6 +4709,20 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_CopyObject) { Function main_1 -> __void () { + VariableDeclStatement{ + VariableConst{ + x_11 + none + undefined + __ptr_none__u32 + { + UnaryOp[not set]{ + address-of + Identifier[not set]{x_1} + } + } + } + } VariableDeclStatement{ VariableConst{ x_2 @@ -4650,7 +4730,10 @@ TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_CopyObject) { undefined __u32 { - Identifier[not set]{x_1} + UnaryOp[not set]{ + indirection + Identifier[not set]{x_11} + } } } }