From 7efea888fac7255e37a74e38ded7d2c2a3c1123e Mon Sep 17 00:00:00 2001 From: David Neto Date: Mon, 8 Feb 2021 16:12:09 +0000 Subject: [PATCH] spirv-reader: instance_index must have u32 store type Fixed: tint:485 Change-Id: I73613ae916b2d86b25470f292e5bf5cd5c7f288b Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/40582 Commit-Queue: David Neto Auto-Submit: David Neto Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 44 +- src/reader/spirv/function.h | 15 + src/reader/spirv/parser_impl.cc | 1 + .../spirv/parser_impl_module_var_test.cc | 378 ++++++++++++++++++ 4 files changed, 432 insertions(+), 6 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 58fa33f322..0767da6d15 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -2040,6 +2040,11 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) { << "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: " @@ -3048,13 +3053,15 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { SkipReason::kPointSizeBuiltinValue; return true; case SkipReason::kSampleIdBuiltinPointer: - case SkipReason::kVertexIndexBuiltinPointer: { + case SkipReason::kVertexIndexBuiltinPointer: + case SkipReason::kInstanceIndexBuiltinPointer: { // The SPIR-V variable is i32, but WGSL requires u32. - auto var_id = parser_impl_.IdForSpecialBuiltIn( - (skip_reason == SkipReason::kSampleIdBuiltinPointer) - ? SpvBuiltInSampleId - : SpvBuiltInVertexIndex); - auto name = namer_.Name(var_id); + 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 expr = TypedExpression{ @@ -3133,6 +3140,28 @@ 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) { @@ -3725,6 +3754,9 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() { 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 = diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index 27ab12a103..b562968df9 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -233,6 +233,11 @@ enum class SkipReason { /// 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, @@ -351,6 +356,9 @@ inline std::ostream& operator<<(std::ostream& o, const DefInfo& di) { case SkipReason::kVertexIndexBuiltinPointer: o << " skip:vertexindex_pointer"; break; + case SkipReason::kInstanceIndexBuiltinPointer: + o << " skip:instanceindex_pointer"; + break; case SkipReason::kSampleMaskInBuiltinPointer: o << " skip:samplemaskin_pointer"; break; @@ -762,6 +770,13 @@ 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.cc b/src/reader/spirv/parser_impl.cc index e4a212eef0..ebd2ce8d01 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -1294,6 +1294,7 @@ ast::Variable* ParserImpl::MakeVariable( return nullptr; case SpvBuiltInSampleId: case SpvBuiltInVertexIndex: + case SpvBuiltInInstanceIndex: // The SPIR-V variable is likely to be signed (because GLSL // requires signed), but WGSL requires unsigned. Handle specially // so we always perform the conversion at load and store. diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc index 443150965c..49e81c3b45 100644 --- a/src/reader/spirv/parser_impl_module_var_test.cc +++ b/src/reader/spirv/parser_impl_module_var_test.cc @@ -3349,6 +3349,384 @@ TEST_F(SpvModuleScopeVarParserTest, VertexIndex_U32_FunctParam) { })")) << module_str; } +// Returns the start of a shader for testing InstanceIndex, +// parameterized by store type of %int or %uint +std::string InstanceIndexPreamble(std::string store_type) { + return R"( + OpCapability Shader + OpMemoryModel Logical Simple + OpEntryPoint Vertex %main "main" %1 + OpDecorate %1 BuiltIn InstanceIndex + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %float = OpTypeFloat 32 + %uint = OpTypeInt 32 0 + %int = OpTypeInt 32 1 + %ptr_ty = OpTypePointer Input )" + + store_type + R"( + %1 = OpVariable %ptr_ty Input +)"; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_Direct) { + const std::string assembly = InstanceIndexPreamble("%int") + R"( + %main = OpFunction %void None %voidfn + %entry = OpLabel + %2 = OpLoad %int %1 + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + 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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct body + EXPECT_THAT(module_str, HasSubstr(R"( + VariableDeclStatement{ + VariableConst{ + x_2 + none + __i32 + { + TypeConstructor[not set]{ + __i32 + Identifier[not set]{x_1} + } + } + } + })")) + << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_CopyObject) { + const std::string assembly = InstanceIndexPreamble("%int") + R"( + %main = OpFunction %void None %voidfn + %entry = OpLabel + %copy_ptr = OpCopyObject %ptr_ty %1 + %2 = OpLoad %int %copy_ptr + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + 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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct body + EXPECT_THAT(module_str, HasSubstr(R"( + VariableDeclStatement{ + VariableConst{ + x_2 + none + __i32 + { + TypeConstructor[not set]{ + __i32 + Identifier[not set]{x_1} + } + } + } + })")) + << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_Load_AccessChain) { + const std::string assembly = InstanceIndexPreamble("%int") + R"( + %main = OpFunction %void None %voidfn + %entry = OpLabel + %copy_ptr = OpAccessChain %ptr_ty %1 + %2 = OpLoad %int %copy_ptr + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + 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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct body + EXPECT_THAT(module_str, HasSubstr(R"( + VariableDeclStatement{ + VariableConst{ + x_2 + none + __i32 + { + TypeConstructor[not set]{ + __i32 + Identifier[not set]{x_1} + } + } + } + })")) + << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_I32_FunctParam) { + const std::string assembly = InstanceIndexPreamble("%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 " + "InstanceIndex builtin, with ID: 1")); +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_Direct) { + const std::string assembly = InstanceIndexPreamble("%uint") + R"( + %main = OpFunction %void None %voidfn + %entry = OpLabel + %2 = OpLoad %uint %1 + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + 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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct body + EXPECT_THAT(module_str, HasSubstr(R"( + VariableDeclStatement{ + VariableConst{ + x_2 + none + __u32 + { + Identifier[not set]{x_1} + } + } + })")) + << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_CopyObject) { + const std::string assembly = InstanceIndexPreamble("%uint") + R"( + %main = OpFunction %void None %voidfn + %entry = OpLabel + %copy_ptr = OpCopyObject %ptr_ty %1 + %2 = OpLoad %uint %copy_ptr + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + 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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct body + EXPECT_THAT(module_str, HasSubstr(R"( + VariableDeclStatement{ + VariableConst{ + x_11 + none + __ptr_in__u32 + { + Identifier[not set]{x_1} + } + } + } + VariableDeclStatement{ + VariableConst{ + x_2 + none + __u32 + { + Identifier[not set]{x_11} + } + } + })")) + << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_Load_AccessChain) { + const std::string assembly = InstanceIndexPreamble("%uint") + R"( + %main = OpFunction %void None %voidfn + %entry = OpLabel + %copy_ptr = OpAccessChain %ptr_ty %1 + %2 = OpLoad %uint %copy_ptr + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + 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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct body + EXPECT_THAT(module_str, HasSubstr(R"( + VariableDeclStatement{ + VariableConst{ + x_2 + none + __u32 + { + Identifier[not set]{x_1} + } + } + })")) + << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, InstanceIndex_U32_FunctParam) { + const std::string assembly = InstanceIndexPreamble("%uint") + R"( + %helper_ty = OpTypeFunction %uint %ptr_ty + %helper = OpFunction %uint None %helper_ty + %param = OpFunctionParameter %ptr_ty + %helper_entry = OpLabel + %3 = OpLoad %uint %param + OpReturnValue %3 + OpFunctionEnd + + %main = OpFunction %void None %voidfn + %entry = OpLabel + %result = OpFunctionCall %uint %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_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"( + Variable{ + Decorations{ + BuiltinDecoration{instance_index} + } + x_1 + in + __u32 + })")); + + // Correct bodies + EXPECT_THAT(module_str, HasSubstr(R"( + Function x_11 -> __u32 + ( + VariableConst{ + x_12 + none + __ptr_in__u32 + } + ) + { + VariableDeclStatement{ + VariableConst{ + x_3 + none + __u32 + { + Identifier[not set]{x_12} + } + } + } + Return{ + { + Identifier[not set]{x_3} + } + } + } + Function main -> __void + StageDecoration{vertex} + () + { + VariableDeclStatement{ + VariableConst{ + x_15 + none + __u32 + { + Call[not set]{ + Identifier[not set]{x_11} + ( + Identifier[not set]{x_1} + ) + } + } + } + } + Return{} + } +})")) << module_str; +} + // TODO(dneto): Test passing pointer to SampleMask as function parameter, // both input case and output case.