From a05e31b5af60df895c6e88bdeddb4bac0077ec64 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Sat, 18 Jun 2022 17:26:40 +0000 Subject: [PATCH] tint/resolver: Clean up parameter resolving Change-Id: If5069575e319a1b04682596c2ab364b414300370 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/93782 Reviewed-by: Dan Sinclair Commit-Queue: Ben Clayton Kokoro: Kokoro --- src/tint/resolver/function_validation_test.cc | 4 +- src/tint/resolver/resolver.cc | 71 +++++++++++-------- src/tint/resolver/resolver.h | 20 ++++-- src/tint/resolver/validator.cc | 16 +---- src/tint/resolver/validator.h | 2 +- 5 files changed, 61 insertions(+), 52 deletions(-) diff --git a/src/tint/resolver/function_validation_test.cc b/src/tint/resolver/function_validation_test.cc index 06817aae89..525195f51a 100644 --- a/src/tint/resolver/function_validation_test.cc +++ b/src/tint/resolver/function_validation_test.cc @@ -718,9 +718,7 @@ TEST_F(ResolverFunctionValidationTest, ParameterStoreType_NonAtomicFree) { Func("f", {bar}, ty.void_(), {}); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "12:34 error: store type of function parameter must be a " - "constructible type"); + EXPECT_EQ(r()->error(), "12:34 error: type of function parameter must be constructible"); } TEST_F(ResolverFunctionValidationTest, ParameterSotreType_AtomicFree) { diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 72ff8bc77f..be1765ce2a 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -311,9 +311,7 @@ sem::Type* Resolver::Type(const ast::Type* ty) { return s; } -sem::Variable* Resolver::Variable(const ast::Variable* v, - bool is_global, - uint32_t index /* = 0 */) { +sem::Variable* Resolver::Variable(const ast::Variable* v, bool is_global) { const sem::Type* storage_ty = nullptr; // If the variable has a declared type, resolve it. @@ -327,7 +325,6 @@ sem::Variable* Resolver::Variable(const ast::Variable* v, auto* as_var = v->As(); auto* as_let = v->As(); auto* as_override = v->As(); - auto* as_param = v->As(); const sem::Expression* rhs = nullptr; @@ -402,29 +399,11 @@ sem::Variable* Resolver::Variable(const ast::Variable* v, } if (!ApplyStorageClassUsageToType(storage_class, const_cast(var_ty), v->source)) { - AddNote(std::string("while instantiating ") + ((as_param) ? "parameter " : "variable ") + - builder_->Symbols().NameFor(v->symbol), + AddNote("while instantiating variable " + builder_->Symbols().NameFor(v->symbol), v->source); return nullptr; } - if (as_param) { - if (auto* ptr = var_ty->As()) { - // For MSL, we push module-scope variables into the entry point as pointer - // parameters, so we also need to handle their store type. - if (!ApplyStorageClassUsageToType( - ptr->StorageClass(), const_cast(ptr->StoreType()), v->source)) { - AddNote("while instantiating parameter " + builder_->Symbols().NameFor(v->symbol), - v->source); - return nullptr; - } - } - auto* param = - builder_->create(as_param, index, var_ty, storage_class, access); - builder_->Sem().Add(as_param, param); - return param; - } - if (is_global) { sem::BindingPoint binding_point; if (as_var) { @@ -457,6 +436,45 @@ sem::Variable* Resolver::Variable(const ast::Variable* v, return local; } +sem::Parameter* Resolver::Parameter(const ast::Parameter* param, uint32_t index) { + auto add_note = [&] { + AddNote("while instantiating parameter " + builder_->Symbols().NameFor(param->symbol), + param->source); + }; + + for (auto* attr : param->attributes) { + Mark(attr); + } + if (!validator_.NoDuplicateAttributes(param->attributes)) { + return nullptr; + } + + sem::Type* ty = Type(param->type); + if (!ty) { + return nullptr; + } + + if (!ApplyStorageClassUsageToType(ast::StorageClass::kNone, ty, param->source)) { + add_note(); + return nullptr; + } + + if (auto* ptr = ty->As()) { + // For MSL, we push module-scope variables into the entry point as pointer + // parameters, so we also need to handle their store type. + if (!ApplyStorageClassUsageToType( + ptr->StorageClass(), const_cast(ptr->StoreType()), param->source)) { + add_note(); + return nullptr; + } + } + + auto* sem = builder_->create(param, index, ty, ast::StorageClass::kNone, + ast::Access::kUndefined); + builder_->Sem().Add(param, sem); + return sem; +} + ast::Access Resolver::DefaultAccessForStorageClass(ast::StorageClass storage_class) { // https://gpuweb.github.io/gpuweb/wgsl/#storage-class switch (storage_class) { @@ -574,15 +592,12 @@ sem::Function* Resolver::Function(const ast::Function* decl) { } } - auto* p = As(Variable(param, false, parameter_index++)); + auto* p = Parameter(param, parameter_index++); if (!p) { return nullptr; } - for (auto* attr : param->attributes) { - Mark(attr); - } - if (!validator_.NoDuplicateAttributes(param->attributes)) { + if (!validator_.Parameter(decl, p)) { return nullptr; } diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h index 5f24169394..e75c952f34 100644 --- a/src/tint/resolver/resolver.h +++ b/src/tint/resolver/resolver.h @@ -290,14 +290,20 @@ class Resolver { /// raised. sem::Struct* Structure(const ast::Struct* str); - /// @returns the semantic info for the variable `var`. If an error is - /// raised, nullptr is returned. - /// @note this method does not resolve the attributes as these are - /// context-dependent (global, local, parameter) - /// @param var the variable to create or return the `VariableInfo` for + /// @returns the semantic info for the variable `v`. If an error is raised, nullptr is + /// returned. + /// @note this method does not resolve the attributes as these are context-dependent (global, + /// local) + /// @param var the variable /// @param is_global true if this is module scope, otherwise function scope - /// @param index the index of the parameter, if this variable is a parameter - sem::Variable* Variable(const ast::Variable* var, bool is_global, uint32_t index = 0); + sem::Variable* Variable(const ast::Variable* var, bool is_global); + + /// @returns the semantic info for the function parameter `param`. If an error is raised, + /// nullptr is returned. + /// @note the caller is expected to validate the parameter + /// @param param the AST parameter + /// @param index the index of the parameter + sem::Parameter* Parameter(const ast::Parameter* param, uint32_t index); /// Records the storage class usage for the given type, and any transient /// dependencies of the type. Validates that the type can be used for the diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 10e76ca16b..e03aa13335 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -739,11 +739,7 @@ bool Validator::Variable(const sem::Variable* v) const { return true; } -bool Validator::FunctionParameter(const ast::Function* func, const sem::Variable* var) const { - if (!Variable(var)) { - return false; - } - +bool Validator::Parameter(const ast::Function* func, const sem::Variable* var) const { auto* decl = var->Declaration(); if (IsValidationDisabled(decl->attributes, ast::DisabledValidation::kFunctionParameter)) { @@ -779,11 +775,11 @@ bool Validator::FunctionParameter(const ast::Function* func, const sem::Variable if (IsPlain(var->Type())) { if (!var->Type()->IsConstructible()) { - AddError("store type of function parameter must be a constructible type", decl->source); + AddError("type of function parameter must be constructible", decl->source); return false; } } else if (!var->Type()->IsAnyOf()) { - AddError("store type of function parameter cannot be " + sem_.TypeNameOf(var->Type()), + AddError("type of function parameter cannot be " + sem_.TypeNameOf(var->Type()), decl->source); return false; } @@ -948,12 +944,6 @@ bool Validator::Function(const sem::Function* func, ast::PipelineStage stage) co return false; } - for (size_t i = 0; i < decl->params.size(); i++) { - if (!FunctionParameter(decl, func->Parameters()[i])) { - return false; - } - } - if (!func->ReturnType()->Is()) { if (!func->ReturnType()->IsConstructible()) { AddError("function return type must be a constructible type", diff --git a/src/tint/resolver/validator.h b/src/tint/resolver/validator.h index 238682f7f4..30dcaf9a93 100644 --- a/src/tint/resolver/validator.h +++ b/src/tint/resolver/validator.h @@ -298,7 +298,7 @@ class Validator { /// @param func the function the variable is for /// @param var the variable to validate /// @returns true on success, false otherwise - bool FunctionParameter(const ast::Function* func, const sem::Variable* var) const; + bool Parameter(const ast::Function* func, const sem::Variable* var) const; /// Validates a return /// @param ret the return statement to validate