Allow sem::GlobalVariable to hold a location.

During some transforms we may move an entry point parameter to
a global variable. This means the global may now have a Location
attached where it isn't permitted by the WGSL spec.

This CL adds a `location` to the GlobalVariable sem value and
populates it in the resolver.

Bug: tint:1633
Change-Id: I684f715fe52d39a0f890fe76c627c1ae543fc746
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101462
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/tint/resolver/attribute_validation_test.cc

@@ -698,12 +698,12 @@ using VariableAttributeTest = TestWithParams;
 TEST_P(VariableAttributeTest, IsValid) {
     auto& params = GetParam();
 
+    auto attrs = createAttributes(Source{{12, 34}}, *this, params.kind);
+    auto* attr = attrs[0];
     if (IsBindingAttribute(params.kind)) {
-        GlobalVar("a", ty.sampler(ast::SamplerKind::kSampler),
+        GlobalVar("a", ty.sampler(ast::SamplerKind::kSampler), attrs);
-                  createAttributes(Source{{12, 34}}, *this, params.kind));
     } else {
-        GlobalVar("a", ty.f32(), ast::StorageClass::kPrivate,
+        GlobalVar("a", ty.f32(), ast::StorageClass::kPrivate, attrs);
-                  createAttributes(Source{{12, 34}}, *this, params.kind));
     }
 
     if (params.should_pass) {
@@ -711,7 +711,8 @@ TEST_P(VariableAttributeTest, IsValid) {
     } else {
         EXPECT_FALSE(r()->Resolve());
         if (!IsBindingAttribute(params.kind)) {
-            EXPECT_EQ(r()->error(), "12:34 error: attribute is not valid for module-scope 'var'");
+            EXPECT_EQ(r()->error(), "12:34 error: attribute '" + attr->Name() +
+                                        "' is not valid for module-scope 'var'");
         }
     }
 }

src/tint/resolver/resolver.cc

@@ -382,7 +382,8 @@ sem::Variable* Resolver::Let(const ast::Let* v, bool is_global) {
     if (is_global) {
         sem = builder_->create<sem::GlobalVariable>(
             v, ty, sem::EvaluationStage::kRuntime, ast::StorageClass::kNone,
-            ast::Access::kUndefined, /* constant_value */ nullptr, sem::BindingPoint{});
+            ast::Access::kUndefined, /* constant_value */ nullptr, sem::BindingPoint{},
+            std::nullopt);
     } else {
         sem = builder_->create<sem::LocalVariable>(v, ty, sem::EvaluationStage::kRuntime,
                                                    ast::StorageClass::kNone,
@@ -437,7 +438,7 @@ sem::Variable* Resolver::Override(const ast::Override* v) {
 
     auto* sem = builder_->create<sem::GlobalVariable>(
         v, ty, sem::EvaluationStage::kOverride, ast::StorageClass::kNone, ast::Access::kUndefined,
-        /* constant_value */ nullptr, sem::BindingPoint{});
+        /* constant_value */ nullptr, sem::BindingPoint{}, std::nullopt);
     sem->SetConstructor(rhs);
 
     if (auto* id_attr = ast::GetAttribute<ast::IdAttribute>(v->attributes)) {
@@ -521,7 +522,7 @@ sem::Variable* Resolver::Const(const ast::Const* c, bool is_global) {
 
     auto* sem = is_global ? static_cast<sem::Variable*>(builder_->create<sem::GlobalVariable>(
                                 c, ty, sem::EvaluationStage::kConstant, ast::StorageClass::kNone,
-                                ast::Access::kUndefined, value, sem::BindingPoint{}))
+                                ast::Access::kUndefined, value, sem::BindingPoint{}, std::nullopt))
                           : static_cast<sem::Variable*>(builder_->create<sem::LocalVariable>(
                                 c, ty, sem::EvaluationStage::kConstant, ast::StorageClass::kNone,
                                 ast::Access::kUndefined, current_statement_, value));
@@ -636,9 +637,15 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) {
             }
             binding_point = {group, binding};
         }
-        sem = builder_->create<sem::GlobalVariable>(var, var_ty, sem::EvaluationStage::kRuntime,
+
-                                                    storage_class, access,
+        std::optional<uint32_t> location;
-                                                    /* constant_value */ nullptr, binding_point);
+        if (auto* attr = ast::GetAttribute<ast::LocationAttribute>(var->attributes)) {
+            location = attr->value;
+        }
+
+        sem = builder_->create<sem::GlobalVariable>(
+            var, var_ty, sem::EvaluationStage::kRuntime, storage_class, access,
+            /* constant_value */ nullptr, binding_point, location);
 
     } else {
         sem = builder_->create<sem::LocalVariable>(var, var_ty, sem::EvaluationStage::kRuntime,

src/tint/resolver/resolver_test.cc

@@ -806,6 +806,18 @@ TEST_F(ResolverTest, Function_Parameters_Locations) {
     EXPECT_EQ(1u, func_sem->Parameters()[2]->Location());
 }
 
+TEST_F(ResolverTest, Function_GlobalVariable_Location) {
+    auto* var = GlobalVar(
+        "my_vec", ty.vec4<f32>(), ast::StorageClass::kIn,
+        utils::Vector{Location(3), Disable(ast::DisabledValidation::kIgnoreStorageClass)});
+
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+
+    auto* sem = Sem().Get<sem::GlobalVariable>(var);
+    ASSERT_NE(sem, nullptr);
+    EXPECT_EQ(3u, sem->Location());
+}
+
 TEST_F(ResolverTest, Function_RegisterInputOutputVariables) {
     auto* s = Structure("S", utils::Vector{Member("m", ty.u32())});
 

src/tint/resolver/validator.cc

@@ -601,7 +601,8 @@ bool Validator::GlobalVariable(
                 if (!attr->IsAnyOf<ast::BindingAttribute, ast::GroupAttribute,
                                    ast::InternalAttribute>() &&
                     (!is_shader_io_attribute || !has_io_storage_class)) {
-                    AddError("attribute is not valid for module-scope 'var'", attr->source);
+                    AddError("attribute '" + attr->Name() + "' is not valid for module-scope 'var'",
+                             attr->source);
                     return false;
                 }
             }

src/tint/sem/variable.cc

@@ -61,9 +61,11 @@ GlobalVariable::GlobalVariable(const ast::Variable* declaration,
                                ast::StorageClass storage_class,
                                ast::Access access,
                                const Constant* constant_value,
-                               sem::BindingPoint binding_point)
+                               sem::BindingPoint binding_point,
+                               std::optional<uint32_t> location)
     : Base(declaration, type, stage, storage_class, access, constant_value),
-      binding_point_(binding_point) {}
+      binding_point_(binding_point),
+      location_(location) {}
 
 GlobalVariable::~GlobalVariable() = default;
 

src/tint/sem/variable.h

@@ -153,13 +153,18 @@ class GlobalVariable final : public Castable<GlobalVariable, Variable> {
     /// @param access the variable access control type
     /// @param constant_value the constant value for the variable. May be null
     /// @param binding_point the optional resource binding point of the variable
+    /// @param location the location value if provided
+    ///
+    /// Note, a GlobalVariable generally doesn't have a `location` in WGSL, as it isn't allowed by
+    /// the spec. The location maybe attached by transforms such as CanonicalizeEntryPointIO.
     GlobalVariable(const ast::Variable* declaration,
                    const sem::Type* type,
                    EvaluationStage stage,
                    ast::StorageClass storage_class,
                    ast::Access access,
                    const Constant* constant_value,
-                   sem::BindingPoint binding_point = {});
+                   sem::BindingPoint binding_point = {},
+                   std::optional<uint32_t> location = std::nullopt);
 
     /// Destructor
     ~GlobalVariable() override;
@@ -173,10 +178,14 @@ class GlobalVariable final : public Castable<GlobalVariable, Variable> {
     /// @returns the pipeline constant ID associated with the variable
     tint::OverrideId OverrideId() const { return override_id_; }
 
+    /// @returns the location value for the parameter, if set
+    std::optional<uint32_t> Location() const { return location_; }
+
   private:
     const sem::BindingPoint binding_point_;
 
     tint::OverrideId override_id_;
+    std::optional<uint32_t> location_;
 };
 
 /// Parameter is a function parameter