[spirv-reader] Only support column-major matrices - drop ColMajor, MatrixStride member decoration - RowMajor matrix decoration is an error Bug: tint:3, tint:99, tint:31 Change-Id: I7eb1ec53813a4b1ada971e8725dc91ffdf97bd43 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/25920 Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 835508d..96ec160 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc
@@ -360,8 +360,20 @@ return result; } +std::string ParserImpl::ShowType(uint32_t type_id) { + if (def_use_mgr_) { + const auto* type_inst = def_use_mgr_->GetDef(type_id); + if (type_inst) { + return type_inst->PrettyPrint(); + } + } + return "SPIR-V type " + std::to_string(type_id); +} + std::unique_ptr<ast::StructMemberDecoration> -ParserImpl::ConvertMemberDecoration(const Decoration& decoration) { +ParserImpl::ConvertMemberDecoration(uint32_t struct_type_id, + uint32_t member_index, + const Decoration& decoration) { if (decoration.empty()) { Fail() << "malformed SPIR-V decoration: it's empty"; return nullptr; @@ -371,7 +383,8 @@ if (decoration.size() != 2) { Fail() << "malformed Offset decoration: expected 1 literal operand, has " - << decoration.size() - 1; + << decoration.size() - 1 << ": member " << member_index << " of " + << ShowType(struct_type_id); return nullptr; } return std::make_unique<ast::StructMemberOffsetDecoration>(decoration[1]); @@ -380,11 +393,33 @@ // TODO(dneto): Drop these for now. // https://github.com/gpuweb/gpuweb/issues/935 return nullptr; + case SpvDecorationColMajor: + // WGSL only supports column major matrices. + return nullptr; + case SpvDecorationRowMajor: + Fail() << "WGSL does not support row-major matrices: can't " + "translate member " + << member_index << " of " << ShowType(struct_type_id); + return nullptr; + case SpvDecorationMatrixStride: { + if (decoration.size() != 2) { + Fail() << "malformed MatrixStride decoration: expected 1 literal " + "operand, has " + << decoration.size() - 1 << ": member " << member_index << " of " + << ShowType(struct_type_id); + return nullptr; + } + // TODO(dneto): Fail if the matrix stride is not allocation size of the + // column vector of the underlying matrix. This would need to unpack + // any levels of array-ness. + return nullptr; + } default: // TODO(dneto): Support the remaining member decorations. break; } - Fail() << "unhandled member decoration: " << decoration[0]; + Fail() << "unhandled member decoration: " << decoration[0] << " on member " + << member_index << " of " << ShowType(struct_type_id); return nullptr; } @@ -748,7 +783,8 @@ Fail() << "unrecognized builtin " << decoration[1]; return nullptr; } else { - auto ast_member_decoration = ConvertMemberDecoration(decoration); + auto ast_member_decoration = + ConvertMemberDecoration(type_id, member_index, decoration); if (!success_) { return nullptr; }
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index 623e1b2..953ff14 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h
@@ -182,14 +182,24 @@ DecorationList GetDecorationsForMember(uint32_t id, uint32_t member_index) const; - /// Converts a SPIR-V decoration. If the decoration is recognized but - /// deliberately dropped, then returns nullptr without a diagnostic. - /// On failure, emits a diagnostic and returns nullptr. + /// Converts a SPIR-V struct member decoration. If the decoration is + /// recognized but deliberately dropped, then returns nullptr without a + /// diagnostic. On failure, emits a diagnostic and returns nullptr. + /// @param struct_type_id the ID of the struct type + /// @param member_index the index of the member /// @param decoration an encoded SPIR-V Decoration /// @returns the corresponding ast::StructuMemberDecoration std::unique_ptr<ast::StructMemberDecoration> ConvertMemberDecoration( + uint32_t struct_type_id, + uint32_t member_index, const Decoration& decoration); + /// Returns a string for the given type. If the type ID is invalid, + /// then the resulting string only names the type ID. + /// @param type_id the SPIR-V ID for the type + /// @returns a string description of the type. + std::string ShowType(uint32_t type_id); + /// Builds the internal representation of the SPIR-V module. /// Assumes the module is somewhat well-formed. Normally you /// would want to validate the SPIR-V module before attempting
diff --git a/src/reader/spirv/parser_impl_convert_member_decoration_test.cc b/src/reader/spirv/parser_impl_convert_member_decoration_test.cc index 19a93a4..e3e4f14 100644 --- a/src/reader/spirv/parser_impl_convert_member_decoration_test.cc +++ b/src/reader/spirv/parser_impl_convert_member_decoration_test.cc
@@ -34,7 +34,7 @@ TEST_F(SpvParserTest, ConvertMemberDecoration_Empty) { auto* p = parser(std::vector<uint32_t>{}); - auto result = p->ConvertMemberDecoration({}); + auto result = p->ConvertMemberDecoration(1, 1, {}); EXPECT_EQ(result.get(), nullptr); EXPECT_THAT(p->error(), Eq("malformed SPIR-V decoration: it's empty")); } @@ -42,27 +42,25 @@ TEST_F(SpvParserTest, ConvertMemberDecoration_OffsetWithoutOperand) { auto* p = parser(std::vector<uint32_t>{}); - auto result = p->ConvertMemberDecoration({SpvDecorationOffset}); + auto result = p->ConvertMemberDecoration(12, 13, {SpvDecorationOffset}); EXPECT_EQ(result.get(), nullptr); - EXPECT_THAT( - p->error(), - Eq("malformed Offset decoration: expected 1 literal operand, has 0")); + EXPECT_THAT(p->error(), Eq("malformed Offset decoration: expected 1 literal " + "operand, has 0: member 13 of SPIR-V type 12")); } TEST_F(SpvParserTest, ConvertMemberDecoration_OffsetWithTooManyOperands) { auto* p = parser(std::vector<uint32_t>{}); - auto result = p->ConvertMemberDecoration({SpvDecorationOffset, 3, 4}); + auto result = p->ConvertMemberDecoration(12, 13, {SpvDecorationOffset, 3, 4}); EXPECT_EQ(result.get(), nullptr); - EXPECT_THAT( - p->error(), - Eq("malformed Offset decoration: expected 1 literal operand, has 2")); + EXPECT_THAT(p->error(), Eq("malformed Offset decoration: expected 1 literal " + "operand, has 2: member 13 of SPIR-V type 12")); } TEST_F(SpvParserTest, ConvertMemberDecoration_Offset) { auto* p = parser(std::vector<uint32_t>{}); - auto result = p->ConvertMemberDecoration({SpvDecorationOffset, 8}); + auto result = p->ConvertMemberDecoration(1, 1, {SpvDecorationOffset, 8}); ASSERT_NE(result.get(), nullptr); EXPECT_TRUE(result->IsOffset()); auto* offset_deco = result->AsOffset(); @@ -74,9 +72,10 @@ TEST_F(SpvParserTest, ConvertMemberDecoration_UnhandledDecoration) { auto* p = parser(std::vector<uint32_t>{}); - auto result = p->ConvertMemberDecoration({12345678}); + auto result = p->ConvertMemberDecoration(12, 13, {12345678}); EXPECT_EQ(result.get(), nullptr); - EXPECT_THAT(p->error(), Eq("unhandled member decoration: 12345678")); + EXPECT_THAT(p->error(), Eq("unhandled member decoration: 12345678 on member " + "13 of SPIR-V type 12")); } } // namespace
diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc index 564784c..3839f42 100644 --- a/src/reader/spirv/parser_impl_module_var_test.cc +++ b/src/reader/spirv/parser_impl_module_var_test.cc
@@ -1336,6 +1336,78 @@ })")) << module_str; } +TEST_F(SpvParserTest, ModuleScopeVar_ColMajorDecoration_Dropped) { + auto* p = parser(test::Assemble(R"( + OpName %myvar "myvar" + OpDecorate %s Block + OpMemberDecorate %s 0 ColMajor + %float = OpTypeFloat 32 + %v2float = OpTypeVector %float 2 + %m3v2float = OpTypeMatrix %v2float 3 + + %s = OpTypeStruct %m3v2float + %ptr_sb_s = OpTypePointer StorageBuffer %s + %myvar = OpVariable %ptr_sb_s StorageBuffer + )")); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + EXPECT_TRUE(p->error().empty()); + const auto module_str = p->module().to_str(); + EXPECT_THAT(module_str, HasSubstr(R"( + Variable{ + myvar + storage_buffer + __alias_S__struct_S + } +S -> __struct_S +})")) << module_str; +} + +TEST_F(SpvParserTest, ModuleScopeVar_MatrixStrideDecoration_Dropped) { + auto* p = parser(test::Assemble(R"( + OpName %myvar "myvar" + OpDecorate %s Block + OpMemberDecorate %s 0 MatrixStride 8 + %float = OpTypeFloat 32 + %v2float = OpTypeVector %float 2 + %m3v2float = OpTypeMatrix %v2float 3 + + %s = OpTypeStruct %m3v2float + %ptr_sb_s = OpTypePointer StorageBuffer %s + %myvar = OpVariable %ptr_sb_s StorageBuffer + )")); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + EXPECT_TRUE(p->error().empty()); + const auto module_str = p->module().to_str(); + EXPECT_THAT(module_str, HasSubstr(R"( + Variable{ + myvar + storage_buffer + __alias_S__struct_S + } +S -> __struct_S +})")) << module_str; +} + +TEST_F(SpvParserTest, ModuleScopeVar_RowMajorDecoration_IsError) { + auto* p = parser(test::Assemble(R"( + OpName %myvar "myvar" + OpDecorate %s Block + OpMemberDecorate %s 0 RowMajor + %float = OpTypeFloat 32 + %v2float = OpTypeVector %float 2 + %m3v2float = OpTypeMatrix %v2float 3 + + %s = OpTypeStruct %m3v2float + %ptr_sb_s = OpTypePointer StorageBuffer %s + %myvar = OpVariable %ptr_sb_s StorageBuffer + )")); + EXPECT_FALSE(p->BuildAndParseInternalModuleExceptFunctions()); + EXPECT_THAT( + p->error(), + Eq(R"(WGSL does not support row-major matrices: can't translate member 0 of %2 = OpTypeStruct %5)")) + << p->error(); +} + } // namespace } // namespace spirv } // namespace reader