From 6cf7f2eca5dc0c06833e213bac5d88ed542c46bc Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Fri, 21 May 2021 14:32:38 +0000 Subject: [PATCH] Remove ast::AccessControl::kInvalid Also spruce up texture validation tests so that they validate error messages. Bug: tint:805 Change-Id: I6c86fc16014b127a7ef8254e5badf9b5bed08623 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/51860 Reviewed-by: Ben Clayton Kokoro: Kokoro Commit-Queue: Antonio Maiorano --- src/ast/access_control.cc | 10 --- src/ast/access_control.h | 3 - src/inspector/inspector.cc | 4 - src/intrinsic_table.cc | 82 +++++++++++++++------ src/resolver/resolver.cc | 51 +++++-------- src/resolver/resolver.h | 6 +- src/resolver/type_validation_test.cc | 30 ++++++-- src/sem/variable.cc | 2 +- src/transform/decompose_storage_access.cc | 2 +- src/writer/hlsl/generator_impl.cc | 17 ++--- src/writer/hlsl/generator_impl_type_test.cc | 38 +++++----- src/writer/spirv/builder.cc | 56 +++++++------- 12 files changed, 162 insertions(+), 139 deletions(-) diff --git a/src/ast/access_control.cc b/src/ast/access_control.cc index 52e3532c6c..635eeb66b5 100644 --- a/src/ast/access_control.cc +++ b/src/ast/access_control.cc @@ -37,9 +37,6 @@ AccessControl::~AccessControl() = default; std::string AccessControl::type_name() const { std::string name = "__access_control_"; switch (access_) { - case ast::AccessControl::kInvalid: - name += "invalid"; - break; case ast::AccessControl::kReadOnly: name += "read_only"; break; @@ -57,9 +54,6 @@ std::string AccessControl::FriendlyName(const SymbolTable& symbols) const { std::ostringstream out; out << "[[access("; switch (access_) { - case ast::AccessControl::kInvalid: - out << "invalid"; - break; case ast::AccessControl::kReadOnly: out << "read"; break; @@ -83,10 +77,6 @@ AccessControl* AccessControl::Clone(CloneContext* ctx) const { std::ostream& operator<<(std::ostream& out, AccessControl::Access access) { switch (access) { - case ast::AccessControl::kInvalid: { - out << "invalid"; - break; - } case ast::AccessControl::kReadOnly: { out << "read_only"; break; diff --git a/src/ast/access_control.h b/src/ast/access_control.h index 23ce43dfb8..fa5f92aaca 100644 --- a/src/ast/access_control.h +++ b/src/ast/access_control.h @@ -28,9 +28,6 @@ class AccessControl : public Castable { public: /// The access control settings enum Access { - /// Invalid - // TODO(crbug.com/tint/805): Remove this. - kInvalid = -1, /// Read only kReadOnly, /// Write only diff --git a/src/inspector/inspector.cc b/src/inspector/inspector.cc index b687b22260..527e0d1bd1 100644 --- a/src/inspector/inspector.cc +++ b/src/inspector/inspector.cc @@ -641,10 +641,6 @@ std::vector Inspector::GetStorageBufferResourceBindingsImpl( auto* var = rsv.first; auto binding_info = rsv.second; - if (var->AccessControl() == ast::AccessControl::kInvalid) { - continue; - } - if (read_only != (var->AccessControl() == ast::AccessControl::kReadOnly)) { continue; } diff --git a/src/intrinsic_table.cc b/src/intrinsic_table.cc index e313d0a262..bcbb5c3af9 100644 --- a/src/intrinsic_table.cc +++ b/src/intrinsic_table.cc @@ -40,7 +40,8 @@ enum class OpenType { enum class OpenNumber { N, // Typically used for vecN M, // Typically used for matNxM - F, // Typically used for texture_storage_2d + F, // Typically used F in for texture_storage_2d + A, // Typically used for A in texture_storage_2d }; /// @return a string of the OpenType symbol `ty` @@ -64,6 +65,8 @@ const char* str(OpenNumber num) { return "M"; case OpenNumber::F: return "F"; + case OpenNumber::A: + return "A"; } return ""; } @@ -516,13 +519,23 @@ class DepthTextureBuilder : public Builder { /// the given texel and channel formats. class StorageTextureBuilder : public Builder { public: - explicit StorageTextureBuilder( - ast::TextureDimension dimensions, - ast::AccessControl::Access access, - OpenNumber texel_format, // a.k.a "image format" - OpenType channel_format) // a.k.a "storage subtype" + StorageTextureBuilder(ast::TextureDimension dimensions, + ast::AccessControl::Access access, + OpenNumber texel_format, // a.k.a "image format" + OpenType channel_format) // a.k.a "storage subtype" : dimensions_(dimensions), access_(access), + access_is_open_num_(false), + texel_format_(texel_format), + channel_format_(channel_format) {} + + StorageTextureBuilder(ast::TextureDimension dimensions, + OpenNumber access, + OpenNumber texel_format, // a.k.a "image format" + OpenType channel_format) // a.k.a "storage subtype" + : dimensions_(dimensions), + access_(access), + access_is_open_num_(true), texel_format_(texel_format), channel_format_(channel_format) {} @@ -531,10 +544,16 @@ class StorageTextureBuilder : public Builder { if (MatchOpenNumber(state, texel_format_, static_cast(tex->image_format()))) { if (MatchOpenType(state, channel_format_, tex->type())) { - // AccessControl::kInvalid means match any - if (access_ != ast::AccessControl::kInvalid && - access_ != tex->access_control()) { - return false; + if (access_is_open_num_) { + if (!MatchOpenNumber( + state, access_.open_num, + static_cast(tex->access_control()))) { + return false; + } + } else { + if (access_.enum_val != tex->access_control()) { + return false; + } } return tex->dim() == dimensions_; @@ -547,9 +566,13 @@ class StorageTextureBuilder : public Builder { sem::Type* Build(BuildState& state) const override { auto texel_format = static_cast(state.open_numbers.at(texel_format_)); + auto access = access_is_open_num_ + ? static_cast( + state.open_numbers.at(access_.open_num)) + : access_.enum_val; auto* channel_format = state.open_types.at(channel_format_); return state.ty_mgr.Get( - dimensions_, texel_format, access_, + dimensions_, texel_format, access, const_cast(channel_format)); } @@ -557,10 +580,10 @@ class StorageTextureBuilder : public Builder { std::stringstream ss; ss << "texture_storage_" << dimensions_ << ""; @@ -569,7 +592,14 @@ class StorageTextureBuilder : public Builder { private: ast::TextureDimension const dimensions_; - ast::AccessControl::Access const access_; + union Access { + Access(OpenNumber in) : open_num(in) {} + Access(ast::AccessControl::Access in) : enum_val(in) {} + + OpenNumber const open_num; + ast::AccessControl::Access const enum_val; + } access_; + bool access_is_open_num_; OpenNumber const texel_format_; OpenType const channel_format_; }; @@ -748,6 +778,14 @@ class Impl : public IntrinsicTable { dimensions, access, texel_format, channel_format); } + Builder* storage_texture(ast::TextureDimension dimensions, + OpenNumber access, + OpenNumber texel_format, + OpenType channel_format) { + return matcher_allocator_.Create( + dimensions, access, texel_format, channel_format); + } + /// @returns a Matcher / Builder that matches an external texture Builder* external_texture() { return matcher_allocator_.Create(); @@ -1079,14 +1117,14 @@ Impl::Impl() { auto* tex_depth_cube = depth_texture(Dim::kCube); auto* tex_depth_cube_array = depth_texture(Dim::kCubeArray); auto* tex_external = external_texture(); - auto* tex_storage_1d_FT = storage_texture( - Dim::k1d, ast::AccessControl::kInvalid, OpenNumber::F, OpenType::T); - auto* tex_storage_2d_FT = storage_texture( - Dim::k2d, ast::AccessControl::kInvalid, OpenNumber::F, OpenType::T); - auto* tex_storage_2d_array_FT = storage_texture( - Dim::k2dArray, ast::AccessControl::kInvalid, OpenNumber::F, OpenType::T); - auto* tex_storage_3d_FT = storage_texture( - Dim::k3d, ast::AccessControl::kInvalid, OpenNumber::F, OpenType::T); + auto* tex_storage_1d_FT = + storage_texture(Dim::k1d, OpenNumber::A, OpenNumber::F, OpenType::T); + auto* tex_storage_2d_FT = + storage_texture(Dim::k2d, OpenNumber::A, OpenNumber::F, OpenType::T); + auto* tex_storage_2d_array_FT = + storage_texture(Dim::k2dArray, OpenNumber::A, OpenNumber::F, OpenType::T); + auto* tex_storage_3d_FT = + storage_texture(Dim::k3d, OpenNumber::A, OpenNumber::F, OpenType::T); auto* tex_storage_ro_1d_FT = storage_texture( Dim::k1d, ast::AccessControl::kReadOnly, OpenNumber::F, OpenType::T); auto* tex_storage_ro_2d_FT = storage_texture( diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 384cfde175..e63800f56c 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -284,7 +284,7 @@ sem::Type* Resolver::Type(const ast::Type* ty) { return builder_->create(); } if (auto* t = ty->As()) { - TINT_SCOPED_ASSIGNMENT(curent_access_control_, t); + TINT_SCOPED_ASSIGNMENT(current_access_control_, t); if (auto* el = Type(t->type())) { return el; } @@ -337,18 +337,15 @@ sem::Type* Resolver::Type(const ast::Type* ty) { } if (auto* t = ty->As()) { if (auto* el = Type(t->type())) { - auto ac = ast::AccessControl::kInvalid; - if (curent_access_control_) { - ac = curent_access_control_->access_control(); - } else { - // TODO(amaiorano): move error about missing access control on storage - // textures here, instead of when variables declared. That way, we'd - // get the error on the alias line (for - // alias>). + if (!current_access_control_) { + diagnostics_.add_error("storage textures must have access control", + t->source()); + return nullptr; } - return builder_->create( - t->dim(), t->image_format(), ac, const_cast(el)); + t->dim(), t->image_format(), + current_access_control_->access_control(), + const_cast(el)); } return nullptr; } @@ -485,11 +482,7 @@ Resolver::VariableInfo* Resolver::Variable(ast::Variable* var, return nullptr; }; - auto access_control = ast::AccessControl::kInvalid; - if (auto* ac = find_first_access_control(var->type())) { - access_control = ac->access_control(); - } - + auto* access_control = find_first_access_control(var->type()); auto* info = variable_infos_.Create(var, const_cast(type), type_name, storage_class, access_control); variable_to_info_.emplace(var, info); @@ -666,7 +659,7 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) { // Its store type must be a host-shareable structure type with block // attribute, satisfying the storage class constraints. - auto* str = info->access_control != ast::AccessControl::kInvalid + auto* str = info->access_control ? info->type->UnwrapRef()->As() : nullptr; @@ -740,7 +733,7 @@ bool Resolver::ValidateVariable(const VariableInfo* info) { if (auto* r = storage_type->As()) { if (r->dim() != ast::TextureDimension::k2d) { - diagnostics_.add_error("Only 2d multisampled textures are supported", + diagnostics_.add_error("only 2d multisampled textures are supported", var->source()); return false; } @@ -754,24 +747,18 @@ bool Resolver::ValidateVariable(const VariableInfo* info) { } if (auto* storage_tex = info->type->UnwrapRef()->As()) { - if (storage_tex->access_control() == ast::AccessControl::kInvalid) { - diagnostics_.add_error("Storage Textures must have access control.", - var->source()); - return false; - } - - if (info->access_control == ast::AccessControl::kReadWrite) { + if (info->access_control->access_control() == + ast::AccessControl::kReadWrite) { diagnostics_.add_error( - "Storage Textures only support Read-Only and Write-Only access " - "control.", + "storage textures only support read-only and write-only access", var->source()); return false; } if (!IsValidStorageTextureDimension(storage_tex->dim())) { diagnostics_.add_error( - "Cube dimensions for storage textures are not " - "supported.", + "cube dimensions for storage textures are not " + "supported", var->source()); return false; } @@ -2548,7 +2535,9 @@ void Resolver::CreateSemanticNodes() const { sem_var = builder_->create(var, info->type, constant_id); } else { sem_var = builder_->create( - var, info->type, info->storage_class, info->access_control); + var, info->type, info->storage_class, + info->access_control ? info->access_control->access_control() + : ast::AccessControl::kReadWrite); } std::vector users; @@ -3235,7 +3224,7 @@ Resolver::VariableInfo::VariableInfo(const ast::Variable* decl, sem::Type* ty, const std::string& tn, ast::StorageClass sc, - ast::AccessControl::Access ac) + const ast::AccessControl* ac) : declaration(decl), type(ty), type_name(tn), diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index 2df47896fc..402e051dbd 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -88,14 +88,14 @@ class Resolver { sem::Type* type, const std::string& type_name, ast::StorageClass storage_class, - ast::AccessControl::Access ac); + const ast::AccessControl* ac); ~VariableInfo(); ast::Variable const* const declaration; sem::Type* type; std::string const type_name; ast::StorageClass storage_class; - ast::AccessControl::Access const access_control; + ast::AccessControl const* const access_control; std::vector users; sem::BindingPoint binding_point; }; @@ -382,7 +382,7 @@ class Resolver { FunctionInfo* current_function_ = nullptr; sem::Statement* current_statement_ = nullptr; - const ast::AccessControl* curent_access_control_ = nullptr; + const ast::AccessControl* current_access_control_ = nullptr; BlockAllocator variable_infos_; BlockAllocator function_infos_; }; diff --git a/src/resolver/type_validation_test.cc b/src/resolver/type_validation_test.cc index 86481e954b..29365ae6ec 100644 --- a/src/resolver/type_validation_test.cc +++ b/src/resolver/type_validation_test.cc @@ -430,7 +430,7 @@ static constexpr DimensionParams dimension_cases[] = { using MultisampledTextureDimensionTest = ResolverTestWithParam; TEST_P(MultisampledTextureDimensionTest, All) { auto& params = GetParam(); - Global("a", ty.multisampled_texture(params.dim, ty.i32()), + Global(Source{{12, 34}}, "a", ty.multisampled_texture(params.dim, ty.i32()), ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), @@ -441,6 +441,8 @@ TEST_P(MultisampledTextureDimensionTest, All) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } else { EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: only 2d multisampled textures are supported"); } } INSTANTIATE_TEST_SUITE_P(ResolverTypeValidationTest, @@ -473,7 +475,7 @@ using MultisampledTextureTypeTest = ResolverTestWithParam; TEST_P(MultisampledTextureTypeTest, All) { auto& params = GetParam(); Global( - "a", + Source{{12, 34}}, "a", ty.multisampled_texture(ast::TextureDimension::k2d, params.type_func(ty)), ast::StorageClass::kNone, nullptr, ast::DecorationList{ @@ -485,6 +487,9 @@ TEST_P(MultisampledTextureTypeTest, All) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } else { EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: texture_multisampled_2d: type must be f32, " + "i32 or u32"); } } INSTANTIATE_TEST_SUITE_P(ResolverTypeValidationTest, @@ -516,7 +521,7 @@ TEST_P(StorageTextureDimensionTest, All) { auto* st = ty.storage_texture(params.dim, ast::ImageFormat::kR32Uint); auto* ac = ty.access(ast::AccessControl::kReadOnly, st); - Global("a", ac, ast::StorageClass::kNone, nullptr, + Global(Source{{12, 34}}, "a", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -526,6 +531,9 @@ TEST_P(StorageTextureDimensionTest, All) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } else { EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + "12:34 error: cube dimensions for storage textures are not supported"); } } INSTANTIATE_TEST_SUITE_P(ResolverTypeValidationTest, @@ -588,7 +596,7 @@ TEST_P(StorageTextureFormatTest, All) { auto* st_a = ty.storage_texture(ast::TextureDimension::k1d, params.format); auto* ac_a = ty.access(ast::AccessControl::kReadOnly, st_a); - Global("a", ac_a, ast::StorageClass::kNone, nullptr, + Global(Source{{12, 34}}, "a", ac_a, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -623,6 +631,10 @@ TEST_P(StorageTextureFormatTest, All) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } else { EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: image format must be one of the texel formats " + "specified for storage textues in " + "https://gpuweb.github.io/gpuweb/wgsl/#texel-formats"); } } INSTANTIATE_TEST_SUITE_P(ResolverTypeValidationTest, @@ -635,7 +647,7 @@ TEST_F(StorageTextureAccessControlTest, MissingAccessControl_Fail) { // [[group(0), binding(0)]] // var a : texture_storage_1d; - auto* st = ty.storage_texture(ast::TextureDimension::k1d, + auto* st = ty.storage_texture(Source{{12, 34}}, ast::TextureDimension::k1d, ast::ImageFormat::kR32Uint); Global("a", st, ast::StorageClass::kNone, nullptr, @@ -645,6 +657,8 @@ TEST_F(StorageTextureAccessControlTest, MissingAccessControl_Fail) { }); EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: storage textures must have access control"); } TEST_F(StorageTextureAccessControlTest, RWAccessControl_Fail) { @@ -653,14 +667,18 @@ TEST_F(StorageTextureAccessControlTest, RWAccessControl_Fail) { auto* st = ty.storage_texture(ast::TextureDimension::k1d, ast::ImageFormat::kR32Uint); + auto* ac = ty.access(ast::AccessControl::kReadWrite, st); - Global("a", st, ast::StorageClass::kNone, nullptr, + Global(Source{{12, 34}}, "a", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), }); EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: storage textures only support read-only and " + "write-only access"); } TEST_F(StorageTextureAccessControlTest, ReadOnlyAccessControl_Pass) { diff --git a/src/sem/variable.cc b/src/sem/variable.cc index 3700c06959..a8cd0119ef 100644 --- a/src/sem/variable.cc +++ b/src/sem/variable.cc @@ -39,7 +39,7 @@ Variable::Variable(const ast::Variable* declaration, : declaration_(declaration), type_(type), storage_class_(ast::StorageClass::kNone), - access_control_(ast::AccessControl::kInvalid), + access_control_(ast::AccessControl::kReadWrite), is_pipeline_constant_(true), constant_id_(constant_id) {} diff --git a/src/transform/decompose_storage_access.cc b/src/transform/decompose_storage_access.cc index 46495a1237..e718b137f2 100644 --- a/src/transform/decompose_storage_access.cc +++ b/src/transform/decompose_storage_access.cc @@ -364,7 +364,7 @@ ast::Type* MaybeCreateASTAccessControl(CloneContext* ctx, const sem::VariableUser* var_user, ast::Type* ty) { if (var_user && - var_user->Variable()->AccessControl() != ast::AccessControl::kInvalid) { + var_user->Variable()->StorageClass() == ast::StorageClass::kStorage) { return ctx->dst->create( var_user->Variable()->AccessControl(), ty); } diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index 626cc77968..5a3621b1b8 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/src/writer/hlsl/generator_impl.cc @@ -249,7 +249,7 @@ bool GeneratorImpl::EmitBitcast(std::ostream& pre, out << "as"; if (!EmitType(out, type, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) { + ast::AccessControl::kReadWrite, "")) { return false; } out << "("; @@ -1309,7 +1309,7 @@ bool GeneratorImpl::EmitTypeConstructor(std::ostream& pre, out << "{"; } else { if (!EmitType(out, type, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) { + ast::AccessControl::kReadWrite, "")) { return false; } out << "("; @@ -1571,7 +1571,7 @@ bool GeneratorImpl::EmitFunctionInternal(std::ostream& out, auto* func = builder_.Sem().Get(func_ast); if (!EmitType(out, func->ReturnType(), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) { + ast::AccessControl::kReadWrite, "")) { return false; } @@ -1750,11 +1750,6 @@ bool GeneratorImpl::EmitEntryPointData( continue; // Global already emitted } - if (var->AccessControl() == ast::AccessControl::kInvalid) { - diagnostics_.add_error("access control type required for storage buffer"); - return false; - } - auto* type = var->Type()->UnwrapRef(); if (!EmitType(out, type, ast::StorageClass::kStorage, var->AccessControl(), "")) { @@ -2129,7 +2124,7 @@ bool GeneratorImpl::EmitZeroValue(std::ostream& out, const sem::Type* type) { out << "0u"; } else if (auto* vec = type->As()) { if (!EmitType(out, type, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) { + ast::AccessControl::kReadWrite, "")) { return false; } ScopedParen sp(out); @@ -2143,7 +2138,7 @@ bool GeneratorImpl::EmitZeroValue(std::ostream& out, const sem::Type* type) { } } else if (auto* mat = type->As()) { if (!EmitType(out, type, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) { + ast::AccessControl::kReadWrite, "")) { return false; } ScopedParen sp(out); @@ -2515,7 +2510,7 @@ bool GeneratorImpl::EmitStructType(std::ostream& out, auto mem_name = builder_.Symbols().NameFor(mem->Declaration()->symbol()); if (!EmitType(out, mem->Type(), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, mem_name)) { + ast::AccessControl::kReadWrite, mem_name)) { return false; } // Array member name will be output with the type diff --git a/src/writer/hlsl/generator_impl_type_test.cc b/src/writer/hlsl/generator_impl_type_test.cc index a4c2b97034..1346cc826a 100644 --- a/src/writer/hlsl/generator_impl_type_test.cc +++ b/src/writer/hlsl/generator_impl_type_test.cc @@ -38,7 +38,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Array) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, program->TypeOf(arr), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "ary")) + ast::AccessControl::kReadWrite, "ary")) << gen.error(); EXPECT_EQ(result(), "bool ary[4]"); } @@ -50,7 +50,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_ArrayOfArray) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, program->TypeOf(arr), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "ary")) + ast::AccessControl::kReadWrite, "ary")) << gen.error(); EXPECT_EQ(result(), "bool ary[5][4]"); } @@ -64,7 +64,7 @@ TEST_F(HlslGeneratorImplTest_Type, GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, program->TypeOf(arr), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "ary")) + ast::AccessControl::kReadWrite, "ary")) << gen.error(); EXPECT_EQ(result(), "bool ary[5][4][1]"); } @@ -76,7 +76,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_ArrayOfArrayOfArray) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, program->TypeOf(arr), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "ary")) + ast::AccessControl::kReadWrite, "ary")) << gen.error(); EXPECT_EQ(result(), "bool ary[6][5][4]"); } @@ -88,7 +88,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Array_WithoutName) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, program->TypeOf(arr), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "bool[4]"); } @@ -100,7 +100,7 @@ TEST_F(HlslGeneratorImplTest_Type, DISABLED_EmitType_RuntimeArray) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, program->TypeOf(arr), ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "ary")) + ast::AccessControl::kReadWrite, "ary")) << gen.error(); EXPECT_EQ(result(), "bool ary[]"); } @@ -111,7 +111,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Bool) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, bool_, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "bool"); } @@ -122,7 +122,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_F32) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, f32, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "float"); } @@ -133,7 +133,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_I32) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, i32, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "int"); } @@ -146,7 +146,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Matrix) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, mat2x3, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "float2x3"); } @@ -159,7 +159,7 @@ TEST_F(HlslGeneratorImplTest_Type, DISABLED_EmitType_Pointer) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, p, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "float*"); } @@ -214,7 +214,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Struct) { auto* sem_s = program->TypeOf(s)->As(); ASSERT_TRUE(gen.EmitType(out, sem_s, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "S"); } @@ -234,7 +234,7 @@ TEST_F(HlslGeneratorImplTest_Type, DISABLED_EmitType_Struct_InjectPadding) { auto* sem_s = program->TypeOf(s)->As(); ASSERT_TRUE(gen.EmitType(out, sem_s, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(gen.result(), R"(struct S { int a; @@ -290,7 +290,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_U32) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, u32, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "uint"); } @@ -302,7 +302,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Vector) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, vec3, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "float3"); } @@ -313,7 +313,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitType_Void) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, void_, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "void"); } @@ -324,7 +324,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitSampler) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, sampler, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "SamplerState"); } @@ -335,7 +335,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitSamplerComparison) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, sampler, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "SamplerComparisonState"); } @@ -532,7 +532,7 @@ TEST_F(HlslGeneratorImplTest_Type, EmitMultisampledTexture) { GeneratorImpl& gen = Build(); ASSERT_TRUE(gen.EmitType(out, s, ast::StorageClass::kNone, - ast::AccessControl::kInvalid, "")) + ast::AccessControl::kReadWrite, "")) << gen.error(); EXPECT_EQ(result(), "Texture2DMS"); } diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index f267f06b5f..e10bab504c 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -784,35 +784,35 @@ bool Builder::GenerateGlobalVariable(ast::Variable* var) { if (var->has_constructor()) { ops.push_back(Operand::Int(init_id)); - } else if (sem->AccessControl() != ast::AccessControl::kInvalid) { - // type is a sem::Struct or a sem::StorageTexture - switch (sem->AccessControl()) { - case ast::AccessControl::kInvalid: - TINT_ICE(builder_.Diagnostics()) << "missing access control"; - break; - case ast::AccessControl::kWriteOnly: - push_annot( - spv::Op::OpDecorate, - {Operand::Int(var_id), Operand::Int(SpvDecorationNonReadable)}); - break; - case ast::AccessControl::kReadOnly: - push_annot( - spv::Op::OpDecorate, - {Operand::Int(var_id), Operand::Int(SpvDecorationNonWritable)}); - break; - case ast::AccessControl::kReadWrite: - break; - } - } else if (!type->Is()) { - // If we don't have a constructor and we're an Output or Private variable, - // then WGSL requires that we zero-initialize. - if (sem->StorageClass() == ast::StorageClass::kPrivate || - sem->StorageClass() == ast::StorageClass::kOutput) { - init_id = GenerateConstantNullIfNeeded(type); - if (init_id == 0) { - return 0; + } else { + if (type->Is() || type->Is()) { + // type is a sem::Struct or a sem::StorageTexture + switch (sem->AccessControl()) { + case ast::AccessControl::kWriteOnly: + push_annot( + spv::Op::OpDecorate, + {Operand::Int(var_id), Operand::Int(SpvDecorationNonReadable)}); + break; + case ast::AccessControl::kReadOnly: + push_annot( + spv::Op::OpDecorate, + {Operand::Int(var_id), Operand::Int(SpvDecorationNonWritable)}); + break; + case ast::AccessControl::kReadWrite: + break; + } + } + if (!type->Is()) { + // If we don't have a constructor and we're an Output or Private + // variable, then WGSL requires that we zero-initialize. + if (sem->StorageClass() == ast::StorageClass::kPrivate || + sem->StorageClass() == ast::StorageClass::kOutput) { + init_id = GenerateConstantNullIfNeeded(type); + if (init_id == 0) { + return 0; + } + ops.push_back(Operand::Int(init_id)); } - ops.push_back(Operand::Int(init_id)); } }