diff --git a/src/reader/spirv/function_memory_test.cc b/src/reader/spirv/function_memory_test.cc index 984ee84d2b..7291980be7 100644 --- a/src/reader/spirv/function_memory_test.cc +++ b/src/reader/spirv/function_memory_test.cc @@ -808,7 +808,7 @@ TEST_F(SpvParserTest, RemapStorageBuffer_TypesAndVarDeclarations) { Variable{ myvar storage_buffer - __struct_S + __access_control_read_write__struct_S })")); } diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 0f43943e6d..d57ee6c2ed 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -51,6 +51,7 @@ #include "src/ast/struct_member.h" #include "src/ast/struct_member_decoration.h" #include "src/ast/struct_member_offset_decoration.h" +#include "src/ast/type/access_control_type.h" #include "src/ast/type/alias_type.h" #include "src/ast/type/array_type.h" #include "src/ast/type/bool_type.h" @@ -391,9 +392,10 @@ ParserImpl::ConvertMemberDecoration(uint32_t struct_type_id, } return std::make_unique(decoration[1]); case SpvDecorationNonReadable: + // WGSL doesn't have a member decoration for this. Silently drop it. + return nullptr; case SpvDecorationNonWritable: - // TODO(dneto): Drop these for now. - // https://github.com/gpuweb/gpuweb/issues/935 + // WGSL doesn't have a member decoration for this. return nullptr; case SpvDecorationColMajor: // WGSL only supports column major matrices. @@ -813,6 +815,7 @@ ast::type::Type* ParserImpl::ConvertType( // Compute members ast::StructMemberList ast_members; const auto members = struct_ty->element_types(); + unsigned num_non_writable_members = 0; for (uint32_t member_index = 0; member_index < members.size(); ++member_index) { const auto member_type_id = type_mgr_->GetId(members[member_index]); @@ -822,6 +825,7 @@ ast::type::Type* ParserImpl::ConvertType( return nullptr; } ast::StructMemberDecorationList ast_member_decorations; + bool is_non_writable = false; for (auto& decoration : GetDecorationsForMember(type_id, member_index)) { if (decoration.empty()) { Fail() << "malformed SPIR-V decoration: it's empty"; @@ -844,6 +848,11 @@ ast::type::Type* ParserImpl::ConvertType( } Fail() << "unrecognized builtin " << decoration[1]; return nullptr; + } else if (decoration[0] == SpvDecorationNonWritable) { + // WGSL doesn't represent individual members as non-writable. Instead, + // apply the ReadOnly access control to the containing struct if all + // the members are non-writable. + is_non_writable = true; } else { auto ast_member_decoration = ConvertMemberDecoration(type_id, member_index, decoration); @@ -855,6 +864,11 @@ ast::type::Type* ParserImpl::ConvertType( } } } + if (is_non_writable) { + // Count a member as non-writable only once, no matter how many + // NonWritable decorations are applied to it. + ++num_non_writable_members; + } const auto member_name = namer_.GetMemberName(type_id, member_index); auto ast_struct_member = std::make_unique( member_name, ast_member_ty, std::move(ast_member_decorations)); @@ -871,6 +885,9 @@ ast::type::Type* ParserImpl::ConvertType( auto* result = ctx_.type_mgr().Get(std::move(ast_struct_type)); id_to_type_[type_id] = result; + if (num_non_writable_members == members.size()) { + read_only_struct_types_.insert(result); + } ast_module_.AddConstructedType(result); return result; } @@ -1058,6 +1075,16 @@ std::unique_ptr ParserImpl::MakeVariable(uint32_t id, Fail() << "internal error: can't make ast::Variable for null type"; return nullptr; } + + if (sc == ast::StorageClass::kStorageBuffer) { + // Apply the access(read) or access(read_write) modifier. + auto access = read_only_struct_types_.count(type) + ? ast::AccessControl::kReadOnly + : ast::AccessControl::kReadWrite; + type = ctx_.type_mgr().Get( + std::make_unique(access, type)); + } + auto ast_var = std::make_unique(namer_.Name(id), sc, type); ast::VariableDecorationList ast_decorations; diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index 555c77f96e..7becccd10c 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h @@ -485,6 +485,9 @@ class ParserImpl : Reader { // and Block decoration. std::unordered_set remap_buffer_block_type_; + // The struct types with only read-only members. + std::unordered_set read_only_struct_types_; + // Maps function_id to a list of entrypoint information std::unordered_map> function_to_ep_info_; diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc index 253ec6a0e3..467d79ac09 100644 --- a/src/reader/spirv/parser_impl_module_var_test.cc +++ b/src/reader/spirv/parser_impl_module_var_test.cc @@ -1203,7 +1203,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_DescriptorSetDecoration_Valid) { } myvar storage_buffer - __struct_S + __access_control_read_write__struct_S })")) << module_str; } @@ -1257,7 +1257,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_BindingDecoration_Valid) { } myvar storage_buffer - __struct_S + __access_control_read_write__struct_S })")) << module_str; } @@ -1292,7 +1292,8 @@ TEST_F(SpvParserTest, "instruction, found '4'.")); } -TEST_F(SpvParserTest, ModuleScopeVar_NonReadableDecoration_DroppedForNow) { +TEST_F(SpvParserTest, + ModuleScopeVar_StructMember_NonReadableDecoration_Dropped) { auto* p = parser(test::Assemble(R"( OpName %myvar "myvar" OpDecorate %strct Block @@ -1301,7 +1302,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_NonReadableDecoration_DroppedForNow) { %ptr_sb_strct = OpTypePointer StorageBuffer %strct %myvar = OpVariable %ptr_sb_strct StorageBuffer )")); - ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error(); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()); EXPECT_TRUE(p->error().empty()); const auto module_str = p->module().to_str(); EXPECT_THAT(module_str, HasSubstr(R"( @@ -1314,36 +1315,9 @@ TEST_F(SpvParserTest, ModuleScopeVar_NonReadableDecoration_DroppedForNow) { Variable{ myvar storage_buffer - __struct_S + __access_control_read_write__struct_S } -})")) << module_str; -} - -TEST_F(SpvParserTest, ModuleScopeVar_NonWritableDecoration_DroppedForNow) { - auto* p = parser(test::Assemble(R"( - OpName %myvar "myvar" - OpDecorate %strct Block - OpMemberDecorate %strct 0 NonWritable -)" + CommonTypes() + R"( - %ptr_sb_strct = OpTypePointer StorageBuffer %strct - %myvar = OpVariable %ptr_sb_strct 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"( - S Struct{ - [[block]] - StructMember{field0: __u32} - StructMember{field1: __f32} - StructMember{field2: __array__u32_2} - } - Variable{ - myvar - storage_buffer - __struct_S - } -})")) << module_str; +)")) << module_str; } TEST_F(SpvParserTest, ModuleScopeVar_ColMajorDecoration_Dropped) { @@ -1370,7 +1344,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_ColMajorDecoration_Dropped) { Variable{ myvar storage_buffer - __struct_S + __access_control_read_write__struct_S } })")) << module_str; } @@ -1399,7 +1373,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_MatrixStrideDecoration_Dropped) { Variable{ myvar storage_buffer - __struct_S + __access_control_read_write__struct_S } })")) << module_str; } @@ -1424,6 +1398,97 @@ TEST_F(SpvParserTest, ModuleScopeVar_RowMajorDecoration_IsError) { << p->error(); } +TEST_F(SpvParserTest, ModuleScopeVar_StorageBuffer_NonWritable_AllMembers) { + // Variable should have access(read) + auto* p = parser(test::Assemble(R"( + OpName %myvar "myvar" + OpDecorate %s Block + OpMemberDecorate %s 0 NonWritable + OpMemberDecorate %s 1 NonWritable + %float = OpTypeFloat 32 + + %s = OpTypeStruct %float %float + %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"( + S Struct{ + [[block]] + StructMember{field0: __f32} + StructMember{field1: __f32} + } + Variable{ + myvar + storage_buffer + __access_control_read_only__struct_S + } +})")) << module_str; +} + +TEST_F(SpvParserTest, ModuleScopeVar_StorageBuffer_NonWritable_NotAllMembers) { + // Variable should have access(read_write) + auto* p = parser(test::Assemble(R"( + OpName %myvar "myvar" + OpDecorate %s Block + OpMemberDecorate %s 0 NonWritable + %float = OpTypeFloat 32 + + %s = OpTypeStruct %float %float + %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"( + S Struct{ + [[block]] + StructMember{field0: __f32} + StructMember{field1: __f32} + } + Variable{ + myvar + storage_buffer + __access_control_read_write__struct_S + } +})")) << module_str; +} + +TEST_F( + SpvParserTest, + ModuleScopeVar_StorageBuffer_NonWritable_NotAllMembers_DuplicatedOnSameMember) { + // Variable should have access(read_write) + auto* p = parser(test::Assemble(R"( + OpName %myvar "myvar" + OpDecorate %s Block + OpMemberDecorate %s 0 NonWritable + OpMemberDecorate %s 0 NonWritable ; same member. Don't double-count it + %float = OpTypeFloat 32 + + %s = OpTypeStruct %float %float + %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"( + S Struct{ + [[block]] + StructMember{field0: __f32} + StructMember{field1: __f32} + } + Variable{ + myvar + storage_buffer + __access_control_read_write__struct_S + } +})")) << module_str; +} + } // namespace } // namespace spirv } // namespace reader