commit fcda15ef67d79bfb4970baeade76b5fb0b2f6cf2
author Ben Clayton <bclayton@google.com> Mon May 10 18:01:51 2021 +0000
committer Commit Bot service account <commit-bot@chromium.org> Mon May 10 18:01:51 2021 +0000
tree 67faafa0158f7317be7b775b3466982a6f9afedd
parent a34fa0ecb7556cf98c04753c89f5d980bb6c467c

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>

src/resolver/resolver.cc

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;