From fcda15ef67d79bfb4970baeade76b5fb0b2f6cf2 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Mon, 10 May 2021 18:01:51 +0000 Subject: [PATCH] Move storage_class validation from wgsl to resolver We don't want the WGSL parser to have to maintain type lookups. If the WGSL language is updated to allow module-scope variables to be declared in any order, then the single-pass approach is going to fail horribly. Instead do the check in the Resovler. With this change, the AST nodes actually contain the correctly declared storage class. Fix up the SPIR-V reader to generate StorageClass::kNone for handle types. Fix all tests. Bug: tint:724 Change-Id: I102e30c9bbef32de40e123c2676ea9a281dee74d Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/50306 Commit-Queue: Ben Clayton Reviewed-by: Antonio Maiorano Reviewed-by: James Price --- src/ast/intrinsic_texture_helper_test.cc | 11 +++-- src/ast/storage_class.cc | 4 ++ src/ast/storage_class.h | 3 +- src/inspector/inspector_test.cc | 15 ++++--- src/reader/spirv/enum_converter.cc | 4 +- src/reader/spirv/enum_converter_test.cc | 4 +- src/reader/spirv/function.cc | 12 +++--- src/reader/spirv/parser_impl.cc | 3 +- .../spirv/parser_impl_convert_type_test.cc | 2 +- src/reader/wgsl/parser_impl.cc | 11 ----- src/reader/wgsl/parser_impl_error_msg_test.cc | 16 -------- .../parser_impl_global_variable_decl_test.cc | 32 --------------- src/resolver/assignment_validation_test.cc | 12 ++---- src/resolver/intrinsic_test.cc | 5 ++- src/resolver/resolver.cc | 40 ++++++++++++++----- src/resolver/resolver.h | 4 +- src/resolver/resolver_test.cc | 21 ++++++++++ src/resolver/type_validation_test.cc | 22 +++++----- src/resolver/validation_test.cc | 22 ++++++++++ src/writer/hlsl/generator_impl_type_test.cc | 6 +-- src/writer/msl/generator_impl_type_test.cc | 2 +- src/writer/spirv/builder.cc | 6 ++- .../spirv/builder_global_variable_test.cc | 8 ++-- src/writer/spirv/builder_intrinsic_test.cc | 12 +++--- src/writer/spirv/builder_type_test.cc | 14 +++---- .../wgsl/generator_impl_global_decl_test.cc | 5 +-- 26 files changed, 154 insertions(+), 142 deletions(-) diff --git a/src/ast/intrinsic_texture_helper_test.cc b/src/ast/intrinsic_texture_helper_test.cc index 3ff61565af..8837ecd247 100644 --- a/src/ast/intrinsic_texture_helper_test.cc +++ b/src/ast/intrinsic_texture_helper_test.cc @@ -158,24 +158,23 @@ ast::Variable* TextureOverloadCase::buildTextureVariable( return b->Global("texture", b->ty.sampled_texture(texture_dimension, buildResultVectorComponentType(b)), - ast::StorageClass::kUniformConstant, nullptr, decos); + ast::StorageClass::kNone, nullptr, decos); case ast::intrinsic::test::TextureKind::kDepth: return b->Global("texture", b->ty.depth_texture(texture_dimension), - ast::StorageClass::kUniformConstant, nullptr, decos); + ast::StorageClass::kNone, nullptr, decos); case ast::intrinsic::test::TextureKind::kMultisampled: return b->Global( "texture", b->ty.multisampled_texture(texture_dimension, buildResultVectorComponentType(b)), - ast::StorageClass::kUniformConstant, nullptr, decos); + ast::StorageClass::kNone, nullptr, decos); case ast::intrinsic::test::TextureKind::kStorage: { auto st = b->ty.storage_texture(texture_dimension, image_format); auto ac = b->ty.access(access_control, st); - return b->Global("texture", ac, ast::StorageClass::kUniformConstant, - nullptr, decos); + return b->Global("texture", ac, ast::StorageClass::kNone, nullptr, decos); } } @@ -190,7 +189,7 @@ ast::Variable* TextureOverloadCase::buildSamplerVariable( b->create(1), }; return b->Global("sampler", b->ty.sampler(sampler_kind), - ast::StorageClass::kUniformConstant, nullptr, decos); + ast::StorageClass::kNone, nullptr, decos); } std::vector TextureOverloadCase::ValidCases() { diff --git a/src/ast/storage_class.cc b/src/ast/storage_class.cc index 13aa6963d0..62e29a4685 100644 --- a/src/ast/storage_class.cc +++ b/src/ast/storage_class.cc @@ -19,6 +19,10 @@ namespace ast { std::ostream& operator<<(std::ostream& out, StorageClass sc) { switch (sc) { + case StorageClass::kInvalid: { + out << "invalid"; + break; + } case StorageClass::kNone: { out << "none"; break; diff --git a/src/ast/storage_class.h b/src/ast/storage_class.h index a6e5983a7a..8a5cace355 100644 --- a/src/ast/storage_class.h +++ b/src/ast/storage_class.h @@ -22,7 +22,8 @@ namespace ast { /// Storage class of a given pointer. enum class StorageClass { - kNone = -1, + kInvalid = -1, + kNone, kInput, kOutput, kUniform, diff --git a/src/inspector/inspector_test.cc b/src/inspector/inspector_test.cc index 0d12013824..e5589c8850 100644 --- a/src/inspector/inspector_test.cc +++ b/src/inspector/inspector_test.cc @@ -316,8 +316,7 @@ class InspectorHelper : public ProgramBuilder { /// @param group the binding/group to use for the storage buffer /// @param binding the binding number to use for the storage buffer void AddSampler(const std::string& name, uint32_t group, uint32_t binding) { - AddBinding(name, sampler_type(), ast::StorageClass::kUniformConstant, group, - binding); + AddBinding(name, sampler_type(), ast::StorageClass::kNone, group, binding); } /// Adds a comparison sampler variable to the program @@ -327,8 +326,8 @@ class InspectorHelper : public ProgramBuilder { void AddComparisonSampler(const std::string& name, uint32_t group, uint32_t binding) { - AddBinding(name, comparison_sampler_type(), - ast::StorageClass::kUniformConstant, group, binding); + AddBinding(name, comparison_sampler_type(), ast::StorageClass::kNone, group, + binding); } /// Generates a SampledTexture appropriate for the params @@ -366,7 +365,7 @@ class InspectorHelper : public ProgramBuilder { ast::Type* type, uint32_t group, uint32_t binding) { - AddBinding(name, type, ast::StorageClass::kUniformConstant, group, binding); + AddBinding(name, type, ast::StorageClass::kNone, group, binding); } /// Adds a multi-sampled texture variable to the program @@ -378,7 +377,7 @@ class InspectorHelper : public ProgramBuilder { ast::Type* type, uint32_t group, uint32_t binding) { - AddBinding(name, type, ast::StorageClass::kUniformConstant, group, binding); + AddBinding(name, type, ast::StorageClass::kNone, group, binding); } void AddGlobalVariable(const std::string& name, ast::Type* type) { @@ -394,7 +393,7 @@ class InspectorHelper : public ProgramBuilder { ast::Type* type, uint32_t group, uint32_t binding) { - AddBinding(name, type, ast::StorageClass::kUniformConstant, group, binding); + AddBinding(name, type, ast::StorageClass::kNone, group, binding); } /// Generates a function that references a specific sampler variable @@ -566,7 +565,7 @@ class InspectorHelper : public ProgramBuilder { typ::Type type, uint32_t group, uint32_t binding) { - AddBinding(name, type, ast::StorageClass::kUniformConstant, group, binding); + AddBinding(name, type, ast::StorageClass::kNone, group, binding); } /// Generates a function that references a storage texture variable. diff --git a/src/reader/spirv/enum_converter.cc b/src/reader/spirv/enum_converter.cc index 2a782485fd..cfcad1b9aa 100644 --- a/src/reader/spirv/enum_converter.cc +++ b/src/reader/spirv/enum_converter.cc @@ -49,7 +49,7 @@ ast::StorageClass EnumConverter::ToStorageClass(const SpvStorageClass sc) { case SpvStorageClassWorkgroup: return ast::StorageClass::kWorkgroup; case SpvStorageClassUniformConstant: - return ast::StorageClass::kUniformConstant; + return ast::StorageClass::kNone; case SpvStorageClassStorageBuffer: return ast::StorageClass::kStorage; case SpvStorageClassImage: @@ -63,7 +63,7 @@ ast::StorageClass EnumConverter::ToStorageClass(const SpvStorageClass sc) { } Fail() << "unknown SPIR-V storage class: " << uint32_t(sc); - return ast::StorageClass::kNone; + return ast::StorageClass::kInvalid; } ast::Builtin EnumConverter::ToBuiltin(SpvBuiltIn b) { diff --git a/src/reader/spirv/enum_converter_test.cc b/src/reader/spirv/enum_converter_test.cc index 7cb09325b3..d19054ea97 100644 --- a/src/reader/spirv/enum_converter_test.cc +++ b/src/reader/spirv/enum_converter_test.cc @@ -143,7 +143,7 @@ INSTANTIATE_TEST_SUITE_P( StorageClassCase{SpvStorageClassWorkgroup, true, ast::StorageClass::kWorkgroup}, StorageClassCase{SpvStorageClassUniformConstant, true, - ast::StorageClass::kUniformConstant}, + ast::StorageClass::kNone}, StorageClassCase{SpvStorageClassStorageBuffer, true, ast::StorageClass::kStorage}, StorageClassCase{SpvStorageClassImage, true, ast::StorageClass::kImage}, @@ -156,7 +156,7 @@ INSTANTIATE_TEST_SUITE_P(EnumConverterBad, SpvStorageClassTest, testing::Values(StorageClassCase{ static_cast(9999), false, - ast::StorageClass::kNone})); + ast::StorageClass::kInvalid})); // Builtin diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index e9c15c9130..1428e7aa0b 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -4005,11 +4005,13 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() { << "pointer defined in function from unknown opcode: " << inst.PrettyPrint(); } - if (info->storage_class == ast::StorageClass::kUniformConstant) { - info->skip = SkipReason::kOpaqueObject; - } } - if (type->AsSampler() || type->AsImage() || type->AsSampledImage()) { + auto* unwrapped = type; + while (auto* ptr = unwrapped->AsPointer()) { + unwrapped = ptr->pointee_type(); + } + if (unwrapped->AsSampler() || unwrapped->AsImage() || + unwrapped->AsSampledImage()) { // Defer code generation until the instruction that actually acts on // the image. info->skip = SkipReason::kOpaqueObject; @@ -4032,7 +4034,7 @@ ast::StorageClass FunctionEmitter::GetStorageClassForPointerValue(uint32_t id) { return ptr->storage_class; } } - return ast::StorageClass::kNone; + return ast::StorageClass::kInvalid; } const Type* FunctionEmitter::RemapStorageClass(const Type* type, diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 06878cf310..b1392a0d6e 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -1060,7 +1060,7 @@ const Type* ParserImpl::ConvertType(uint32_t type_id, } auto ast_storage_class = enum_converter_.ToStorageClass(storage_class); - if (ast_storage_class == ast::StorageClass::kNone) { + if (ast_storage_class == ast::StorageClass::kInvalid) { Fail() << "SPIR-V pointer type with ID " << type_id << " has invalid storage class " << static_cast(storage_class); @@ -1236,6 +1236,7 @@ bool ParserImpl::EmitModuleScopeVariables() { continue; } switch (enum_converter_.ToStorageClass(spirv_storage_class)) { + case ast::StorageClass::kNone: case ast::StorageClass::kInput: case ast::StorageClass::kOutput: case ast::StorageClass::kUniform: diff --git a/src/reader/spirv/parser_impl_convert_type_test.cc b/src/reader/spirv/parser_impl_convert_type_test.cc index 3b32374947..ea46b0ffb5 100644 --- a/src/reader/spirv/parser_impl_convert_type_test.cc +++ b/src/reader/spirv/parser_impl_convert_type_test.cc @@ -692,7 +692,7 @@ TEST_F(SpvParserTest, ConvertType_PointerUniformConstant) { auto* ptr_ty = type->As(); EXPECT_NE(ptr_ty, nullptr); EXPECT_TRUE(ptr_ty->type->Is()); - EXPECT_EQ(ptr_ty->storage_class, ast::StorageClass::kUniformConstant); + EXPECT_EQ(ptr_ty->storage_class, ast::StorageClass::kNone); EXPECT_TRUE(p->error().empty()); } diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc index 3ea562a3a5..9aba063c93 100644 --- a/src/reader/wgsl/parser_impl.cc +++ b/src/reader/wgsl/parser_impl.cc @@ -564,17 +564,6 @@ Maybe ParserImpl::variable_decl() { if (decl.errored) return Failure::kErrored; - if (decl->type && decl->type->UnwrapAll()->is_handle()) { - // handle types implicitly have the `UniformConstant` storage class. - if (explicit_sc.matched) { - return add_error( - explicit_sc.source, - decl->type->UnwrapAll()->FriendlyName(builder_.Symbols()) + - " variables must not have a storage class"); - } - sc = ast::StorageClass::kUniformConstant; - } - return VarDeclInfo{decl->source, decl->name, sc, decl->type}; } diff --git a/src/reader/wgsl/parser_impl_error_msg_test.cc b/src/reader/wgsl/parser_impl_error_msg_test.cc index b56f6fe8ee..c9c7c833d0 100644 --- a/src/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/reader/wgsl/parser_impl_error_msg_test.cc @@ -1102,22 +1102,6 @@ TEST_F(ParserImplErrorTest, GlobalDeclVarVectorMissingType) { " ^\n"); } -TEST_F(ParserImplErrorTest, GlobalDeclSamplerExplicitStorageClass) { - EXPECT( - "var x : sampler;", - "test.wgsl:1:5 error: sampler variables must not have a storage class\n" - "var x : sampler;\n" - " ^^^^^^^\n"); -} - -TEST_F(ParserImplErrorTest, GlobalDeclTextureExplicitStorageClass) { - EXPECT("var x : [[access(read)]] texture_1d;", - "test.wgsl:1:5 error: texture_1d variables must not have a " - "storage class\n" - "var x : [[access(read)]] texture_1d;\n" - " ^^^^^^^\n"); -} - TEST_F(ParserImplErrorTest, IfStmtMissingLParen) { EXPECT("fn f() { if true) {} }", "test.wgsl:1:13 error: expected '('\n" diff --git a/src/reader/wgsl/parser_impl_global_variable_decl_test.cc b/src/reader/wgsl/parser_impl_global_variable_decl_test.cc index cb469a7bec..7887b02e2c 100644 --- a/src/reader/wgsl/parser_impl_global_variable_decl_test.cc +++ b/src/reader/wgsl/parser_impl_global_variable_decl_test.cc @@ -168,38 +168,6 @@ TEST_F(ParserImplTest, GlobalVariableDecl_InvalidVariableDecl) { EXPECT_EQ(p->error(), "1:5: invalid storage class for variable decoration"); } -TEST_F(ParserImplTest, GlobalVariableDecl_SamplerImplicitStorageClass) { - auto p = parser("var s : sampler;"); - auto decos = p->decoration_list(); - EXPECT_FALSE(decos.errored); - EXPECT_FALSE(decos.matched); - auto e = p->global_variable_decl(decos.value); - ASSERT_FALSE(p->has_error()) << p->error(); - EXPECT_FALSE(e.errored); - EXPECT_TRUE(e.matched); - ASSERT_NE(e.value, nullptr); - - EXPECT_EQ(e->symbol(), p->builder().Symbols().Get("s")); - EXPECT_TRUE(e->type()->Is()); - EXPECT_EQ(e->declared_storage_class(), ast::StorageClass::kUniformConstant); -} - -TEST_F(ParserImplTest, GlobalVariableDecl_TextureImplicitStorageClass) { - auto p = parser("var s : [[access(read)]] texture_1d;"); - auto decos = p->decoration_list(); - EXPECT_FALSE(decos.errored); - EXPECT_FALSE(decos.matched); - auto e = p->global_variable_decl(decos.value); - ASSERT_FALSE(p->has_error()) << p->error(); - EXPECT_FALSE(e.errored); - EXPECT_TRUE(e.matched); - ASSERT_NE(e.value, nullptr); - - EXPECT_EQ(e->symbol(), p->builder().Symbols().Get("s")); - EXPECT_TRUE(e->type()->UnwrapAll()->Is()); - EXPECT_EQ(e->declared_storage_class(), ast::StorageClass::kUniformConstant); -} - TEST_F(ParserImplTest, GlobalVariableDecl_StorageClassIn_Deprecated) { auto p = parser("[[location(0)]] var a : f32"); auto f = p->function_header(); diff --git a/src/resolver/assignment_validation_test.cc b/src/resolver/assignment_validation_test.cc index 52782a1ff3..f469a06580 100644 --- a/src/resolver/assignment_validation_test.cc +++ b/src/resolver/assignment_validation_test.cc @@ -244,19 +244,15 @@ TEST_F(ResolverAssignmentValidationTest, AssignFromPointer_Fail) { return ty.access(ast::AccessControl::kReadOnly, tex_type); }; - auto* var_a = Var("a", make_type(), ast::StorageClass::kFunction); - auto* var_b = Var("b", make_type(), ast::StorageClass::kFunction); + auto* var_a = Global("a", make_type(), ast::StorageClass::kNone); + auto* var_b = Global("b", make_type(), ast::StorageClass::kNone); - auto* lhs = Expr("a"); - auto* rhs = Expr("b"); - - auto* assign = Assign(Source{{12, 34}}, lhs, rhs); - WrapInFunction(Decl(var_a), Decl(var_b), assign); + WrapInFunction(Assign(Source{{12, 34}}, var_a, var_b)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), "12:34 error v-000x: invalid assignment: right-hand-side is not " - "storable: ptr>"); } diff --git a/src/resolver/intrinsic_test.cc b/src/resolver/intrinsic_test.cc index 876fa1d4d1..58811f6fbe 100644 --- a/src/resolver/intrinsic_test.cc +++ b/src/resolver/intrinsic_test.cc @@ -243,7 +243,10 @@ class ResolverIntrinsicTest_TextureOperation void add_call_param(std::string name, typ::Type type, ast::ExpressionList* call_params) { - Global(name, type, ast::StorageClass::kInput); + ast::StorageClass storage_class = type->UnwrapAll()->is_handle() + ? ast::StorageClass::kNone + : ast::StorageClass::kPrivate; + Global(name, type, storage_class); call_params->push_back(Expr(name)); } typ::Type subtype(Texture type) { diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 3e863c6167..8f4007626b 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -500,7 +500,7 @@ bool Resolver::GlobalVariable(ast::Variable* var) { return false; } - if (!ApplyStorageClassUsageToType(var->declared_storage_class(), info->type, + if (!ApplyStorageClassUsageToType(info->storage_class, info->type, var->source())) { diagnostics_.add_note("while instantiating variable " + builder_->Symbols().NameFor(var->symbol()), @@ -577,11 +577,12 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) { break; } - return ValidateVariable(info->declaration); + return ValidateVariable(info); } -bool Resolver::ValidateVariable(const ast::Variable* var) { - auto* type = variable_to_info_[var]->type; +bool Resolver::ValidateVariable(const VariableInfo* info) { + auto* var = info->declaration; + auto* type = info->type; if (auto* r = type->As()) { if (r->IsRuntimeSized()) { diagnostics_.add_error( @@ -643,11 +644,23 @@ bool Resolver::ValidateVariable(const ast::Variable* var) { } } + if (type->UnwrapAll()->is_handle() && + var->declared_storage_class() != ast::StorageClass::kNone) { + // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables + // If the store type is a texture type or a sampler type, then the variable + // declaration must not have a storage class decoration. The storage class + // will always be handle. + diagnostics_.add_error("variables of type '" + info->type_name + + "' must not have a storage class", + var->source()); + return false; + } + return true; } -bool Resolver::ValidateParameter(const ast::Variable* param) { - return ValidateVariable(param); +bool Resolver::ValidateParameter(const VariableInfo* info) { + return ValidateVariable(info); } bool Resolver::ValidateFunction(const ast::Function* func, @@ -662,7 +675,7 @@ bool Resolver::ValidateFunction(const ast::Function* func, } for (auto* param : func->params()) { - if (!ValidateParameter(param)) { + if (!ValidateParameter(variable_to_info_.at(param))) { return false; } } @@ -2060,7 +2073,7 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) { variable_stack_.set(var->symbol(), info); current_block_->decls.push_back(var); - if (!ValidateVariable(var)) { + if (!ValidateVariable(info)) { return false; } @@ -2833,7 +2846,16 @@ Resolver::VariableInfo::VariableInfo(const ast::Variable* decl, : declaration(decl), type(ctype), type_name(tn), - storage_class(decl->declared_storage_class()) {} + storage_class(decl->declared_storage_class()) { + if (storage_class == ast::StorageClass::kNone && + type->UnwrapAll()->is_handle()) { + // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables + // If the store type is a texture type or a sampler type, then the variable + // declaration must not have a storage class decoration. The storage class + // will always be handle. + storage_class = ast::StorageClass::kUniformConstant; + } +} Resolver::VariableInfo::~VariableInfo() = default; diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index 378927bc79..c537eae1a5 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -230,11 +230,11 @@ class Resolver { bool ValidateGlobalVariable(const VariableInfo* var); bool ValidateMatrixConstructor(const ast::TypeConstructorExpression* ctor, const sem::Matrix* matrix_type); - bool ValidateParameter(const ast::Variable* param); + bool ValidateParameter(const VariableInfo* info); bool ValidateReturn(const ast::ReturnStatement* ret); bool ValidateStructure(const sem::Struct* str); bool ValidateSwitch(const ast::SwitchStatement* s); - bool ValidateVariable(const ast::Variable* param); + bool ValidateVariable(const VariableInfo* info); bool ValidateVectorConstructor(const ast::TypeConstructorExpression* ctor, const sem::Vector* vec_type); diff --git a/src/resolver/resolver_test.cc b/src/resolver/resolver_test.cc index fe8d221946..323434812f 100644 --- a/src/resolver/resolver_test.cc +++ b/src/resolver/resolver_test.cc @@ -1458,6 +1458,27 @@ TEST_F(ResolverTest, StorageClass_SetsIfMissing) { EXPECT_EQ(Sem().Get(var)->StorageClass(), ast::StorageClass::kFunction); } +TEST_F(ResolverTest, StorageClass_SetForSampler) { + auto t = ty.sampler(ast::SamplerKind::kSampler); + auto* var = Global("var", t, ast::StorageClass::kNone); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + + EXPECT_EQ(Sem().Get(var)->StorageClass(), + ast::StorageClass::kUniformConstant); +} + +TEST_F(ResolverTest, StorageClass_SetForTexture) { + auto t = ty.sampled_texture(ast::TextureDimension::k1d, ty.f32()); + auto ac = ty.access(ast::AccessControl::Access::kReadOnly, t); + auto* var = Global("var", ac, ast::StorageClass::kNone); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + + EXPECT_EQ(Sem().Get(var)->StorageClass(), + ast::StorageClass::kUniformConstant); +} + TEST_F(ResolverTest, StorageClass_DoesNotSetOnConst) { auto* var = Const("var", ty.i32(), Construct(ty.i32())); auto* stmt = Decl(var); diff --git a/src/resolver/type_validation_test.cc b/src/resolver/type_validation_test.cc index b5dfcc1ea7..f67192504b 100644 --- a/src/resolver/type_validation_test.cc +++ b/src/resolver/type_validation_test.cc @@ -551,7 +551,7 @@ using MultisampledTextureDimensionTest = ResolverTestWithParam; TEST_P(MultisampledTextureDimensionTest, All) { auto& params = GetParam(); Global("a", ty.multisampled_texture(params.dim, ty.i32()), - ast::StorageClass::kUniformConstant, nullptr, + ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -595,7 +595,7 @@ TEST_P(MultisampledTextureTypeTest, All) { Global( "a", ty.multisampled_texture(ast::TextureDimension::k2d, params.type_func(ty)), - ast::StorageClass::kUniformConstant, nullptr, + ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -636,7 +636,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::kUniformConstant, nullptr, + Global("a", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -708,7 +708,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::kUniformConstant, nullptr, + Global("a", ac_a, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -716,7 +716,7 @@ TEST_P(StorageTextureFormatTest, All) { auto st_b = ty.storage_texture(ast::TextureDimension::k2d, params.format); auto ac_b = ty.access(ast::AccessControl::kReadOnly, st_b); - Global("b", ac_b, ast::StorageClass::kUniformConstant, nullptr, + Global("b", ac_b, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(1), @@ -725,7 +725,7 @@ TEST_P(StorageTextureFormatTest, All) { auto st_c = ty.storage_texture(ast::TextureDimension::k2dArray, params.format); auto ac_c = ty.access(ast::AccessControl::kReadOnly, st_c); - Global("c", ac_c, ast::StorageClass::kUniformConstant, nullptr, + Global("c", ac_c, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(2), @@ -733,7 +733,7 @@ TEST_P(StorageTextureFormatTest, All) { auto st_d = ty.storage_texture(ast::TextureDimension::k3d, params.format); auto ac_d = ty.access(ast::AccessControl::kReadOnly, st_d); - Global("d", ac_d, ast::StorageClass::kUniformConstant, nullptr, + Global("d", ac_d, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(3), @@ -758,7 +758,7 @@ TEST_F(StorageTextureAccessControlTest, MissingAccessControl_Fail) { auto st = ty.storage_texture(ast::TextureDimension::k1d, ast::ImageFormat::kR32Uint); - Global("a", st, ast::StorageClass::kUniformConstant, nullptr, + Global("a", st, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -775,7 +775,7 @@ TEST_F(StorageTextureAccessControlTest, RWAccessControl_Fail) { ast::ImageFormat::kR32Uint); auto ac = ty.access(ast::AccessControl::kReadWrite, st); - Global("a", ac, ast::StorageClass::kUniformConstant, nullptr, + Global("a", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -792,7 +792,7 @@ TEST_F(StorageTextureAccessControlTest, ReadOnlyAccessControl_Pass) { ast::ImageFormat::kR32Uint); auto ac = ty.access(ast::AccessControl::kReadOnly, st); - Global("a", ac, ast::StorageClass::kUniformConstant, nullptr, + Global("a", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), @@ -809,7 +809,7 @@ TEST_F(StorageTextureAccessControlTest, WriteOnlyAccessControl_Pass) { ast::ImageFormat::kR32Uint); auto ac = ty.access(ast::AccessControl::kWriteOnly, st); - Global("a", ac, ast::StorageClass::kUniformConstant, nullptr, + Global("a", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(0), create(0), diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc index 7889f8d588..ba3dde3d3c 100644 --- a/src/resolver/validation_test.cc +++ b/src/resolver/validation_test.cc @@ -375,6 +375,28 @@ TEST_F(ResolverValidationTest, StorageClass_NonFunctionClassError) { "error: function variable has a non-function storage class"); } +TEST_F(ResolverValidationTest, StorageClass_SamplerExplicitStorageClass) { + auto t = ty.sampler(ast::SamplerKind::kSampler); + Global(Source{{12, 34}}, "var", t, ast::StorageClass::kUniformConstant); + + EXPECT_FALSE(r()->Resolve()); + + EXPECT_EQ( + r()->error(), + R"(12:34 error: variables of type 'sampler' must not have a storage class)"); +} + +TEST_F(ResolverValidationTest, StorageClass_TextureExplicitStorageClass) { + auto t = ty.sampled_texture(ast::TextureDimension::k1d, ty.f32()); + Global(Source{{12, 34}}, "var", t, ast::StorageClass::kUniformConstant); + + EXPECT_FALSE(r()->Resolve()) << r()->error(); + + EXPECT_EQ( + r()->error(), + R"(12:34 error: variables of type 'texture_1d' must not have a storage class)"); +} + TEST_F(ResolverValidationTest, Expr_MemberAccessor_VectorSwizzle_BadChar) { Global("my_vec", ty.vec3(), ast::StorageClass::kInput); diff --git a/src/writer/hlsl/generator_impl_type_test.cc b/src/writer/hlsl/generator_impl_type_test.cc index 0905be086a..6354625bd6 100644 --- a/src/writer/hlsl/generator_impl_type_test.cc +++ b/src/writer/hlsl/generator_impl_type_test.cc @@ -335,7 +335,7 @@ TEST_P(HlslDepthTexturesTest, Emit) { auto t = ty.depth_texture(params.dim); - Global("tex", t, ast::StorageClass::kUniformConstant, nullptr, + Global("tex", t, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(1), create(2), @@ -394,7 +394,7 @@ TEST_P(HlslSampledTexturesTest, Emit) { } auto t = ty.sampled_texture(params.dim, datatype); - Global("tex", t, ast::StorageClass::kUniformConstant, nullptr, + Global("tex", t, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(1), create(2), @@ -536,7 +536,7 @@ TEST_P(HlslStorageTexturesTest, Emit) { : ast::AccessControl::kWriteOnly, t); - Global("tex", ac, ast::StorageClass::kUniformConstant, nullptr, + Global("tex", ac, ast::StorageClass::kNone, nullptr, ast::DecorationList{ create(1), create(2), diff --git a/src/writer/msl/generator_impl_type_test.cc b/src/writer/msl/generator_impl_type_test.cc index a726ff7b76..80c64c4e77 100644 --- a/src/writer/msl/generator_impl_type_test.cc +++ b/src/writer/msl/generator_impl_type_test.cc @@ -745,7 +745,7 @@ TEST_P(MslStorageTexturesTest, Emit) { auto ac = ty.access(params.ro ? ast::AccessControl::kReadOnly : ast::AccessControl::kWriteOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); GeneratorImpl& gen = Build(); diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index 570d131f51..ee09323077 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -769,8 +769,8 @@ bool Builder::GenerateGlobalVariable(ast::Variable* var) { if (auto* builtin = deco->As()) { push_annot(spv::Op::OpDecorate, {Operand::Int(var_id), Operand::Int(SpvDecorationBuiltIn), - Operand::Int(ConvertBuiltin(builtin->value(), - var->declared_storage_class()))}); + Operand::Int( + ConvertBuiltin(builtin->value(), sem->StorageClass()))}); } else if (auto* location = deco->As()) { push_annot(spv::Op::OpDecorate, {Operand::Int(var_id), Operand::Int(SpvDecorationLocation), @@ -3243,6 +3243,8 @@ bool Builder::GenerateVectorType(const sem::Vector* vec, SpvStorageClass Builder::ConvertStorageClass(ast::StorageClass klass) const { switch (klass) { + case ast::StorageClass::kInvalid: + return SpvStorageClassMax; case ast::StorageClass::kInput: return SpvStorageClassInput; case ast::StorageClass::kOutput: diff --git a/src/writer/spirv/builder_global_variable_test.cc b/src/writer/spirv/builder_global_variable_test.cc index dadc9b40be..4c28e9ffdf 100644 --- a/src/writer/spirv/builder_global_variable_test.cc +++ b/src/writer/spirv/builder_global_variable_test.cc @@ -526,7 +526,7 @@ TEST_F(BuilderTest, GlobalVar_TextureStorageReadOnly) { auto ac = ty.access(ast::AccessControl::kReadOnly, type); - auto* var_a = Global("a", ac, ast::StorageClass::kUniformConstant); + auto* var_a = Global("a", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -549,7 +549,7 @@ TEST_F(BuilderTest, GlobalVar_TextureStorageWriteOnly) { auto ac = ty.access(ast::AccessControl::kWriteOnly, type); - auto* var_a = Global("a", ac, ast::StorageClass::kUniformConstant); + auto* var_a = Global("a", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -573,12 +573,12 @@ TEST_F(BuilderTest, GlobalVar_TextureStorageWithDifferentAccess) { auto type_a = ty.access(ast::AccessControl::kReadOnly, ty.storage_texture(ast::TextureDimension::k2d, ast::ImageFormat::kR32Uint)); - auto* var_a = Global("a", type_a, ast::StorageClass::kUniformConstant); + auto* var_a = Global("a", type_a, ast::StorageClass::kNone); auto type_b = ty.access(ast::AccessControl::kWriteOnly, ty.storage_texture(ast::TextureDimension::k2d, ast::ImageFormat::kR32Uint)); - auto* var_b = Global("b", type_b, ast::StorageClass::kUniformConstant); + auto* var_b = Global("b", type_b, ast::StorageClass::kNone); spirv::Builder& b = Build(); diff --git a/src/writer/spirv/builder_intrinsic_test.cc b/src/writer/spirv/builder_intrinsic_test.cc index 911d2430e7..9e353c1765 100644 --- a/src/writer/spirv/builder_intrinsic_test.cc +++ b/src/writer/spirv/builder_intrinsic_test.cc @@ -373,9 +373,9 @@ TEST_F(IntrinsicBuilderTest, Call_TextureSampleCompare_Twice) { auto s = ty.sampler(ast::SamplerKind::kComparisonSampler); auto t = ty.depth_texture(ast::TextureDimension::k2d); - auto* tex = Global("texture", t, ast::StorageClass::kInput); + auto* tex = Global("texture", t, ast::StorageClass::kNone); - auto* sampler = Global("sampler", s, ast::StorageClass::kInput); + auto* sampler = Global("sampler", s, ast::StorageClass::kNone); auto* expr1 = Call("textureSampleCompare", "texture", "sampler", vec2(1.0f, 2.0f), 2.0f); @@ -397,11 +397,11 @@ TEST_F(IntrinsicBuilderTest, Call_TextureSampleCompare_Twice) { EXPECT_EQ(DumpInstructions(b.types()), R"(%4 = OpTypeFloat 32 %3 = OpTypeImage %4 2D 1 0 0 1 Unknown -%2 = OpTypePointer Input %3 -%1 = OpVariable %2 Input +%2 = OpTypePointer UniformConstant %3 +%1 = OpVariable %2 UniformConstant %7 = OpTypeSampler -%6 = OpTypePointer Input %7 -%5 = OpVariable %6 Input +%6 = OpTypePointer UniformConstant %7 +%5 = OpVariable %6 UniformConstant %11 = OpTypeSampledImage %3 %13 = OpTypeVector %4 2 %14 = OpConstant %4 1 diff --git a/src/writer/spirv/builder_type_test.cc b/src/writer/spirv/builder_type_test.cc index bf158d9b4e..477007b897 100644 --- a/src/writer/spirv/builder_type_test.cc +++ b/src/writer/spirv/builder_type_test.cc @@ -816,7 +816,7 @@ TEST_F(BuilderTest_Type, StorageTexture_Generate_1d) { ast::ImageFormat::kR32Float); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -832,7 +832,7 @@ TEST_F(BuilderTest_Type, StorageTexture_Generate_2d) { ast::ImageFormat::kR32Float); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -848,7 +848,7 @@ TEST_F(BuilderTest_Type, StorageTexture_Generate_2dArray) { ast::ImageFormat::kR32Float); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -864,7 +864,7 @@ TEST_F(BuilderTest_Type, StorageTexture_Generate_3d) { ast::ImageFormat::kR32Float); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -881,7 +881,7 @@ TEST_F(BuilderTest_Type, ast::ImageFormat::kR32Float); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -898,7 +898,7 @@ TEST_F(BuilderTest_Type, ast::ImageFormat::kR32Sint); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); @@ -915,7 +915,7 @@ TEST_F(BuilderTest_Type, ast::ImageFormat::kR32Uint); auto ac = ty.access(ast::AccessControl::kReadOnly, s); - Global("test_var", ac, ast::StorageClass::kInput); + Global("test_var", ac, ast::StorageClass::kNone); spirv::Builder& b = Build(); diff --git a/src/writer/wgsl/generator_impl_global_decl_test.cc b/src/writer/wgsl/generator_impl_global_decl_test.cc index 491cad1655..a77acee31f 100644 --- a/src/writer/wgsl/generator_impl_global_decl_test.cc +++ b/src/writer/wgsl/generator_impl_global_decl_test.cc @@ -101,8 +101,7 @@ TEST_F(WgslGeneratorImplTest, Emit_GlobalsInterleaved) { } TEST_F(WgslGeneratorImplTest, Emit_Global_Sampler) { - Global("s", ty.sampler(ast::SamplerKind::kSampler), - ast::StorageClass::kUniformConstant); + Global("s", ty.sampler(ast::SamplerKind::kSampler), ast::StorageClass::kNone); GeneratorImpl& gen = Build(); @@ -115,7 +114,7 @@ TEST_F(WgslGeneratorImplTest, Emit_Global_Sampler) { TEST_F(WgslGeneratorImplTest, Emit_Global_Texture) { auto st = ty.sampled_texture(ast::TextureDimension::k1d, ty.f32()); Global("t", ty.access(ast::AccessControl::kReadOnly, st), - ast::StorageClass::kUniformConstant); + ast::StorageClass::kNone); GeneratorImpl& gen = Build();