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 <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/resolver/assignment_validation_test.cc b/src/resolver/assignment_validation_test.cc
index 52782a1..f469a06 100644
--- a/src/resolver/assignment_validation_test.cc
+++ b/src/resolver/assignment_validation_test.cc
@@ -244,19 +244,15 @@
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<function, [[access(read)]] "
+ "storable: ptr<uniform_constant, [[access(read)]] "
"texture_storage_1d<rgba8unorm>>");
}
diff --git a/src/resolver/intrinsic_test.cc b/src/resolver/intrinsic_test.cc
index 876fa1d..58811f6 100644
--- a/src/resolver/intrinsic_test.cc
+++ b/src/resolver/intrinsic_test.cc
@@ -243,7 +243,10 @@
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 3e863c6..8f40076 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -500,7 +500,7 @@
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 @@
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<sem::Array>()) {
if (r->IsRuntimeSized()) {
diagnostics_.add_error(
@@ -643,11 +644,23 @@
}
}
+ 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 @@
}
for (auto* param : func->params()) {
- if (!ValidateParameter(param)) {
+ if (!ValidateParameter(variable_to_info_.at(param))) {
return false;
}
}
@@ -2060,7 +2073,7 @@
variable_stack_.set(var->symbol(), info);
current_block_->decls.push_back(var);
- if (!ValidateVariable(var)) {
+ if (!ValidateVariable(info)) {
return false;
}
@@ -2833,7 +2846,16 @@
: 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 378927b..c537eae 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -230,11 +230,11 @@
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 fe8d221..3234348 100644
--- a/src/resolver/resolver_test.cc
+++ b/src/resolver/resolver_test.cc
@@ -1458,6 +1458,27 @@
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 b5dfcc1..f671925 100644
--- a/src/resolver/type_validation_test.cc
+++ b/src/resolver/type_validation_test.cc
@@ -551,7 +551,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -595,7 +595,7 @@
Global(
"a",
ty.multisampled_texture(ast::TextureDimension::k2d, params.type_func(ty)),
- ast::StorageClass::kUniformConstant, nullptr,
+ ast::StorageClass::kNone, nullptr,
ast::DecorationList{
create<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -636,7 +636,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -708,7 +708,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -716,7 +716,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(1),
@@ -725,7 +725,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(2),
@@ -733,7 +733,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(3),
@@ -758,7 +758,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -775,7 +775,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -792,7 +792,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
@@ -809,7 +809,7 @@
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<ast::BindingDecoration>(0),
create<ast::GroupDecoration>(0),
diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc
index 7889f8d..ba3dde3 100644
--- a/src/resolver/validation_test.cc
+++ b/src/resolver/validation_test.cc
@@ -375,6 +375,28 @@
"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<f32>' must not have a storage class)");
+}
+
TEST_F(ResolverValidationTest, Expr_MemberAccessor_VectorSwizzle_BadChar) {
Global("my_vec", ty.vec3<f32>(), ast::StorageClass::kInput);
