From 23cf74c30ea598dd18682eb041217f0e34bd46ad Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Wed, 7 Sep 2022 20:05:54 +0000 Subject: [PATCH] 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 Reviewed-by: Ben Clayton --- .../resolver/attribute_validation_test.cc | 11 ++++++----- src/tint/resolver/resolver.cc | 19 +++++++++++++------ src/tint/resolver/resolver_test.cc | 12 ++++++++++++ src/tint/resolver/validator.cc | 3 ++- src/tint/sem/variable.cc | 6 ++++-- src/tint/sem/variable.h | 11 ++++++++++- 6 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc index 7f573a8e76..6203728ae0 100644 --- a/src/tint/resolver/attribute_validation_test.cc +++ b/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), - createAttributes(Source{{12, 34}}, *this, params.kind)); + GlobalVar("a", ty.sampler(ast::SamplerKind::kSampler), attrs); } else { - GlobalVar("a", ty.f32(), ast::StorageClass::kPrivate, - createAttributes(Source{{12, 34}}, *this, params.kind)); + GlobalVar("a", ty.f32(), ast::StorageClass::kPrivate, attrs); } 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'"); } } } diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index fd0d821bc2..8775168b87 100644 --- a/src/tint/resolver/resolver.cc +++ b/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( 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(v, ty, sem::EvaluationStage::kRuntime, ast::StorageClass::kNone, @@ -437,7 +438,7 @@ sem::Variable* Resolver::Override(const ast::Override* v) { auto* sem = builder_->create( 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(v->attributes)) { @@ -521,7 +522,7 @@ sem::Variable* Resolver::Const(const ast::Const* c, bool is_global) { auto* sem = is_global ? static_cast(builder_->create( c, ty, sem::EvaluationStage::kConstant, ast::StorageClass::kNone, - ast::Access::kUndefined, value, sem::BindingPoint{})) + ast::Access::kUndefined, value, sem::BindingPoint{}, std::nullopt)) : static_cast(builder_->create( 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(var, var_ty, sem::EvaluationStage::kRuntime, - storage_class, access, - /* constant_value */ nullptr, binding_point); + + std::optional location; + if (auto* attr = ast::GetAttribute(var->attributes)) { + location = attr->value; + } + + sem = builder_->create( + var, var_ty, sem::EvaluationStage::kRuntime, storage_class, access, + /* constant_value */ nullptr, binding_point, location); } else { sem = builder_->create(var, var_ty, sem::EvaluationStage::kRuntime, diff --git a/src/tint/resolver/resolver_test.cc b/src/tint/resolver/resolver_test.cc index 523dd89908..3708774d4e 100644 --- a/src/tint/resolver/resolver_test.cc +++ b/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(), ast::StorageClass::kIn, + utils::Vector{Location(3), Disable(ast::DisabledValidation::kIgnoreStorageClass)}); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + + auto* sem = Sem().Get(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())}); diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index caca32ee8e..022f0db50c 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -601,7 +601,8 @@ bool Validator::GlobalVariable( if (!attr->IsAnyOf() && (!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; } } diff --git a/src/tint/sem/variable.cc b/src/tint/sem/variable.cc index 67e79455d5..6dcec638c4 100644 --- a/src/tint/sem/variable.cc +++ b/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 location) : Base(declaration, type, stage, storage_class, access, constant_value), - binding_point_(binding_point) {} + binding_point_(binding_point), + location_(location) {} GlobalVariable::~GlobalVariable() = default; diff --git a/src/tint/sem/variable.h b/src/tint/sem/variable.h index a0eb75e2af..5ea70b937d 100644 --- a/src/tint/sem/variable.h +++ b/src/tint/sem/variable.h @@ -153,13 +153,18 @@ class GlobalVariable final : public Castable { /// @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 location = std::nullopt); /// Destructor ~GlobalVariable() override; @@ -173,10 +178,14 @@ class GlobalVariable final : public Castable { /// @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 Location() const { return location_; } + private: const sem::BindingPoint binding_point_; tint::OverrideId override_id_; + std::optional location_; }; /// Parameter is a function parameter