Resolver: improve array handling
type::Array was the only type with special handling in that we'd resolve
all array types first in ResolverInternal, then proceed with resolving
the AST in order of declaration. This CL removes this special handling,
and instead, we now process Arrays as we process ast::Variables of
array type. This change also allows us to pass down a Source location
for validation messages when processing Arrays.
Updated some Builder tests that weren't creating a variable of the array
type they declared.
Bug: tint:707
Change-Id: I8483b3a979bc1e5e04feb1ca4d281e96e9e654be
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47825
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index a18b481..dfcabea 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -178,16 +178,6 @@
}
bool Resolver::ResolveInternal() {
- // Process non-user-defined types (arrays). The rest will be processed in
- // order of declaration below.
- for (auto* ty : builder_->Types()) {
- if (auto* arr = ty->As<type::Array>()) {
- if (!Array(arr)) {
- return false;
- }
- }
- }
-
// Process everything else in the order they appear in the module. This is
// necessary for validation of use-before-declaration.
for (auto* decl : builder_->AST().GlobalDeclarations()) {
@@ -196,10 +186,6 @@
if (!Structure(str)) {
return false;
}
- } else if (auto* arr = decl->As<type::Array>()) {
- if (!Array(arr)) {
- return false;
- }
}
} else if (auto* func = decl->As<ast::Function>()) {
if (!Function(func)) {
@@ -209,12 +195,37 @@
if (!GlobalVariable(var)) {
return false;
}
+ } else {
+ TINT_UNREACHABLE(diagnostics_)
+ << "unhandled global declaration: " << decl->TypeInfo().name;
+ return false;
}
}
return true;
}
+Resolver::VariableInfo* Resolver::Variable(ast::Variable* var,
+ type::Type* type /*=nullptr*/) {
+ auto it = variable_to_info_.find(var);
+ if (it != variable_to_info_.end()) {
+ return it->second;
+ }
+
+ auto* ctype = Canonical(type ? type : var->declared_type());
+ auto* info = variable_infos_.Create(var, ctype);
+ variable_to_info_.emplace(var, info);
+
+ // Resolve variable's type
+ if (auto* arr = info->type->As<type::Array>()) {
+ if (!Array(arr, var->source())) {
+ return nullptr;
+ }
+ }
+
+ return info;
+}
+
bool Resolver::GlobalVariable(ast::Variable* var) {
if (variable_stack_.has(var->symbol())) {
diagnostics_.add_error("v-0011",
@@ -224,7 +235,10 @@
return false;
}
- auto* info = CreateVariableInfo(var);
+ auto* info = Variable(var);
+ if (!info) {
+ return false;
+ }
variable_stack_.set_global(var->symbol(), info);
if (!var->is_const() && info->storage_class == ast::StorageClass::kNone) {
@@ -535,7 +549,10 @@
variable_stack_.push_scope();
for (auto* param : func->params()) {
- auto* param_info = CreateVariableInfo(param);
+ auto* param_info = Variable(param);
+ if (!param_info) {
+ return false;
+ }
variable_stack_.set(param->symbol(), param_info);
func_info->parameters.emplace_back(param_info);
@@ -1565,7 +1582,12 @@
}
}
- auto* info = CreateVariableInfo(var);
+ auto* info = Variable(var, type);
+ if (!info) {
+ return false;
+ }
+ // TODO(amaiorano): Remove this and fix tests. We're overriding the semantic
+ // type stored in info->type here with a possibly non-canonicalized type.
info->type = type;
variable_stack_.set(var->symbol(), info);
current_block_->decls.push_back(var);
@@ -1597,12 +1619,6 @@
return true;
}
-Resolver::VariableInfo* Resolver::CreateVariableInfo(ast::Variable* var) {
- auto* info = variable_infos_.Create(var, Canonical(var->declared_type()));
- variable_to_info_.emplace(var, info);
- return info;
-}
-
type::Type* Resolver::TypeOf(ast::Expression* expr) {
auto it = expr_info_.find(expr);
if (it != expr_info_.end()) {
@@ -1730,7 +1746,8 @@
bool Resolver::DefaultAlignAndSize(type::Type* ty,
uint32_t& align,
- uint32_t& size) {
+ uint32_t& size,
+ const Source& source) {
static constexpr uint32_t vector_size[] = {
/* padding */ 0,
/* padding */ 0,
@@ -1779,7 +1796,8 @@
}
return false;
} else if (cty->Is<type::Array>()) {
- if (auto* sem = Array(ty->UnwrapAliasIfNeeded()->As<type::Array>())) {
+ if (auto* sem =
+ Array(ty->UnwrapAliasIfNeeded()->As<type::Array>(), source)) {
align = sem->Align();
size = sem->Size();
return true;
@@ -1790,7 +1808,7 @@
return false;
}
-const semantic::Array* Resolver::Array(type::Array* arr) {
+const semantic::Array* Resolver::Array(type::Array* arr, const Source& source) {
if (auto* sem = builder_->Sem().Get(arr)) {
// Semantic info already constructed for this array type
return sem;
@@ -1800,15 +1818,16 @@
auto* el_ty = arr->type();
if (!IsStorable(el_ty)) {
builder_->Diagnostics().add_error(
- std::string(el_ty->FriendlyName(builder_->Symbols())) +
- " cannot be used as an element type of an array");
+ el_ty->FriendlyName(builder_->Symbols()) +
+ " cannot be used as an element type of an array",
+ source);
return nullptr;
}
auto create_semantic = [&](uint32_t stride) -> semantic::Array* {
uint32_t el_align = 0;
uint32_t el_size = 0;
- if (!DefaultAlignAndSize(arr->type(), el_align, el_size)) {
+ if (!DefaultAlignAndSize(arr->type(), el_align, el_size, source)) {
return nullptr;
}
@@ -1831,7 +1850,7 @@
// Calculate implicit stride
uint32_t el_align = 0;
uint32_t el_size = 0;
- if (!DefaultAlignAndSize(el_ty, el_align, el_size)) {
+ if (!DefaultAlignAndSize(el_ty, el_align, el_size, source)) {
return nullptr;
}
@@ -1933,7 +1952,7 @@
uint32_t offset = struct_size;
uint32_t align = 0;
uint32_t size = 0;
- if (!DefaultAlignAndSize(member->type(), align, size)) {
+ if (!DefaultAlignAndSize(member->type(), align, size, member->source())) {
return nullptr;
}
@@ -2189,7 +2208,7 @@
bool Resolver::ApplyStorageClassUsageToType(ast::StorageClass sc,
type::Type* ty,
- Source usage) {
+ const Source& usage) {
ty = ty->UnwrapIfNeeded();
if (auto* str = ty->As<type::Struct>()) {
diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h
index d4f73fc..ee942dc 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -251,12 +251,21 @@
/// @returns the semantic information for the array `arr`, building it if it
/// hasn't been constructed already. If an error is raised, nullptr is
/// returned.
- const semantic::Array* Array(type::Array*);
+ /// @param arr the Array to get semantic information for
+ /// @param source the Source of the ast node with this array as its type
+ const semantic::Array* Array(type::Array* arr, const Source& source);
/// @returns the StructInfo for the structure `str`, building it if it hasn't
/// been constructed already. If an error is raised, nullptr is returned.
StructInfo* Structure(type::Struct* str);
+ /// @returns the VariableInfo for the variable `var`, building it if it hasn't
+ /// been constructed already. If an error is raised, nullptr is returned.
+ /// @param var the variable to create or return the `VariableInfo` for
+ /// @param type optional type of `var` to use instead of
+ /// `var->declared_type()`. For type inference.
+ VariableInfo* Variable(ast::Variable* var, type::Type* type = nullptr);
+
/// 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
/// given storage class, erroring if it cannot.
@@ -267,14 +276,16 @@
/// @returns true on success, false on error
bool ApplyStorageClassUsageToType(ast::StorageClass sc,
type::Type* ty,
- Source usage);
+ const Source& usage);
/// @param align the output default alignment in bytes for the type `ty`
/// @param size the output default size in bytes for the type `ty`
+ /// @param source the Source of the variable declaration of type `ty`
/// @returns true on success, false on error
- bool DefaultAlignAndSize(type::Type* ty, uint32_t& align, uint32_t& size);
-
- VariableInfo* CreateVariableInfo(ast::Variable*);
+ bool DefaultAlignAndSize(type::Type* ty,
+ uint32_t& align,
+ uint32_t& size,
+ const Source& source);
/// @returns the resolved type of the ast::Expression `expr`
/// @param expr the expression
diff --git a/src/writer/spirv/builder_type_test.cc b/src/writer/spirv/builder_type_test.cc
index 09541b5..fbd1645 100644
--- a/src/writer/spirv/builder_type_test.cc
+++ b/src/writer/spirv/builder_type_test.cc
@@ -59,6 +59,7 @@
TEST_F(BuilderTest_Type, GenerateRuntimeArray) {
auto* ary = ty.array(ty.i32(), 0);
+ Global("a", ary, ast::StorageClass::kInput);
spirv::Builder& b = Build();
@@ -73,6 +74,7 @@
TEST_F(BuilderTest_Type, ReturnsGeneratedRuntimeArray) {
auto* ary = ty.array(ty.i32(), 0);
+ Global("a", ary, ast::StorageClass::kInput);
spirv::Builder& b = Build();
@@ -87,6 +89,7 @@
TEST_F(BuilderTest_Type, GenerateArray) {
auto* ary = ty.array(ty.i32(), 4);
+ Global("a", ary, ast::StorageClass::kInput);
spirv::Builder& b = Build();
@@ -103,6 +106,7 @@
TEST_F(BuilderTest_Type, GenerateArray_WithStride) {
auto* ary = ty.array(ty.i32(), 4, 16u);
+ Global("a", ary, ast::StorageClass::kInput);
spirv::Builder& b = Build();
@@ -122,6 +126,7 @@
TEST_F(BuilderTest_Type, ReturnsGeneratedArray) {
auto* ary = ty.array(ty.i32(), 4);
+ Global("a", ary, ast::StorageClass::kInput);
spirv::Builder& b = Build();
