validation: Fix array storage class validation
We weren't recursing into array element types, which meant that we
weren't validating nested arrays or buffers whose top-level store-type
was an array.
Change-Id: Ib897b36e0b5c3de3dc67c4f60805411c014cd914
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/77561
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h
index 89c2808..309cb8a 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -108,8 +108,8 @@
/// Describes the context in which a variable is declared
enum class VariableKind { kParameter, kLocal, kGlobal };
- std::set<std::pair<const sem::Struct*, ast::StorageClass>>
- valid_struct_storage_layouts_;
+ std::set<std::pair<const sem::Type*, ast::StorageClass>>
+ valid_type_storage_layouts_;
/// Structure holding semantic information about a block (i.e. scope), such as
/// parent block and variables declared in the block.
@@ -292,9 +292,9 @@
const sem::Array* arr_type);
bool ValidateTextureIntrinsicFunction(const sem::Call* call);
bool ValidateNoDuplicateDecorations(const ast::DecorationList& decorations);
- // sem::Struct is assumed to have at least one member
- bool ValidateStorageClassLayout(const sem::Struct* type,
- ast::StorageClass sc);
+ bool ValidateStorageClassLayout(const sem::Type* type,
+ ast::StorageClass sc,
+ Source source);
bool ValidateStorageClassLayout(const sem::Variable* var);
/// @returns true if the decoration list contains a
diff --git a/src/resolver/resolver_validation.cc b/src/resolver/resolver_validation.cc
index 8864a26..f3e8302 100644
--- a/src/resolver/resolver_validation.cc
+++ b/src/resolver/resolver_validation.cc
@@ -229,8 +229,9 @@
return true;
}
-bool Resolver::ValidateStorageClassLayout(const sem::Struct* str,
- ast::StorageClass sc) {
+bool Resolver::ValidateStorageClassLayout(const sem::Type* store_ty,
+ ast::StorageClass sc,
+ Source source) {
// https://gpuweb.github.io/gpuweb/wgsl/#storage-class-layout-constraints
auto is_uniform_struct_or_array = [sc](const sem::Type* ty) {
@@ -255,110 +256,125 @@
return builder_->Symbols().NameFor(sm->Declaration()->symbol);
};
+ // Cache result of type + storage class pair.
+ if (!valid_type_storage_layouts_.emplace(store_ty, sc).second) {
+ return true;
+ }
+
if (!ast::IsHostShareable(sc)) {
return true;
}
- for (size_t i = 0; i < str->Members().size(); ++i) {
- auto* const m = str->Members()[i];
- uint32_t required_align = required_alignment_of(m->Type());
+ if (auto* str = store_ty->As<sem::Struct>()) {
+ for (size_t i = 0; i < str->Members().size(); ++i) {
+ auto* const m = str->Members()[i];
+ uint32_t required_align = required_alignment_of(m->Type());
- // Validate that member is at a valid byte offset
- if (m->Offset() % required_align != 0) {
- AddError("the offset of a struct member of type '" +
- m->Type()->UnwrapRef()->FriendlyName(builder_->Symbols()) +
- "' in storage class '" + ast::ToString(sc) +
- "' must be a multiple of " + std::to_string(required_align) +
- " bytes, but '" + member_name_of(m) +
- "' is currently at offset " + std::to_string(m->Offset()) +
- ". Consider setting @align(" +
- std::to_string(required_align) + ") on this member",
- m->Declaration()->source);
-
- AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
- str->Declaration()->source);
-
- if (auto* member_str = m->Type()->As<sem::Struct>()) {
- AddNote("and layout of struct member:\n" +
- member_str->Layout(builder_->Symbols()),
- member_str->Declaration()->source);
+ // Recurse into the member type.
+ if (!ValidateStorageClassLayout(m->Type(), sc,
+ m->Declaration()->type->source)) {
+ AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
+ str->Declaration()->source);
+ return false;
}
- return false;
- }
-
- // For uniform buffers, validate that the number of bytes between the
- // previous member of type struct and the current is a multiple of 16 bytes.
- auto* const prev_member = (i == 0) ? nullptr : str->Members()[i - 1];
- if (prev_member && is_uniform_struct(prev_member->Type())) {
- const uint32_t prev_to_curr_offset = m->Offset() - prev_member->Offset();
- if (prev_to_curr_offset % 16 != 0) {
- AddError(
- "uniform storage requires that the number of bytes between the "
- "start of the previous member of type struct and the current "
- "member be a multiple of 16 bytes, but there are currently " +
- std::to_string(prev_to_curr_offset) + " bytes between '" +
- member_name_of(prev_member) + "' and '" + member_name_of(m) +
- "'. Consider setting @align(16) on this member",
- m->Declaration()->source);
+ // Validate that member is at a valid byte offset
+ if (m->Offset() % required_align != 0) {
+ AddError("the offset of a struct member of type '" +
+ m->Type()->UnwrapRef()->FriendlyName(builder_->Symbols()) +
+ "' in storage class '" + ast::ToString(sc) +
+ "' must be a multiple of " +
+ std::to_string(required_align) + " bytes, but '" +
+ member_name_of(m) + "' is currently at offset " +
+ std::to_string(m->Offset()) +
+ ". Consider setting @align(" +
+ std::to_string(required_align) + ") on this member",
+ m->Declaration()->source);
AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
str->Declaration()->source);
- auto* prev_member_str = prev_member->Type()->As<sem::Struct>();
- AddNote("and layout of previous member struct:\n" +
- prev_member_str->Layout(builder_->Symbols()),
- prev_member_str->Declaration()->source);
+ if (auto* member_str = m->Type()->As<sem::Struct>()) {
+ AddNote("and layout of struct member:\n" +
+ member_str->Layout(builder_->Symbols()),
+ member_str->Declaration()->source);
+ }
+
return false;
}
- }
- // For uniform buffer array members, validate that array elements are
- // aligned to 16 bytes
- if (auto* arr = m->Type()->As<sem::Array>()) {
- if (sc == ast::StorageClass::kUniform) {
- // We already validated that this array member is itself aligned to 16
- // bytes above, so we only need to validate that stride is a multiple of
- // 16 bytes.
- if (arr->Stride() % 16 != 0) {
- // Since WGSL has no stride attribute, try to provide a useful hint
- // for how the shader author can resolve the issue.
- std::string hint;
- if (arr->ElemType()->is_scalar()) {
- hint =
- "Consider using a vector or struct as the element type "
- "instead.";
- } else if (auto* vec = arr->ElemType()->As<sem::Vector>();
- vec && vec->type()->Size() == 4) {
- hint = "Consider using a vec4 instead.";
- } else if (arr->ElemType()->Is<sem::Struct>()) {
- hint =
- "Consider using the @size attribute on the last struct member.";
- } else {
- hint =
- "Consider wrapping the element type in a struct and using the "
- "@size attribute.";
- }
+ // For uniform buffers, validate that the number of bytes between the
+ // previous member of type struct and the current is a multiple of 16
+ // bytes.
+ auto* const prev_member = (i == 0) ? nullptr : str->Members()[i - 1];
+ if (prev_member && is_uniform_struct(prev_member->Type())) {
+ const uint32_t prev_to_curr_offset =
+ m->Offset() - prev_member->Offset();
+ if (prev_to_curr_offset % 16 != 0) {
AddError(
- "uniform storage requires that array elements be aligned to 16 "
- "bytes, but array element alignment of '" +
- member_name_of(m) + "' is currently " +
- std::to_string(arr->Stride()) + ". " + hint,
- m->Declaration()->type->source);
+ "uniform storage requires that the number of bytes between the "
+ "start of the previous member of type struct and the current "
+ "member be a multiple of 16 bytes, but there are currently " +
+ std::to_string(prev_to_curr_offset) + " bytes between '" +
+ member_name_of(prev_member) + "' and '" + member_name_of(m) +
+ "'. Consider setting @align(16) on this member",
+ m->Declaration()->source);
+
AddNote("see layout of struct:\n" + str->Layout(builder_->Symbols()),
str->Declaration()->source);
+
+ auto* prev_member_str = prev_member->Type()->As<sem::Struct>();
+ AddNote("and layout of previous member struct:\n" +
+ prev_member_str->Layout(builder_->Symbols()),
+ prev_member_str->Declaration()->source);
return false;
}
}
}
+ }
- // If member is struct, recurse
- if (auto* str_member = m->Type()->As<sem::Struct>()) {
- // Cache result of struct + storage class pair
- if (valid_struct_storage_layouts_.emplace(str_member, sc).second) {
- if (!ValidateStorageClassLayout(str_member, sc)) {
- return false;
+ // For uniform buffer array members, validate that array elements are
+ // aligned to 16 bytes
+ if (auto* arr = store_ty->As<sem::Array>()) {
+ // Recurse into the element type.
+ // TODO(crbug.com/tint/1388): Ideally we'd pass the source for nested
+ // element type here, but we can't easily get that from the semantic node.
+ // We should consider recursing through the AST type nodes instead.
+ if (!ValidateStorageClassLayout(arr->ElemType(), sc, source)) {
+ return false;
+ }
+
+ if (sc == ast::StorageClass::kUniform) {
+ // We already validated that this array member is itself aligned to 16
+ // bytes above, so we only need to validate that stride is a multiple
+ // of 16 bytes.
+ if (arr->Stride() % 16 != 0) {
+ // Since WGSL has no stride attribute, try to provide a useful hint
+ // for how the shader author can resolve the issue.
+ std::string hint;
+ if (arr->ElemType()->is_scalar()) {
+ hint =
+ "Consider using a vector or struct as the element type "
+ "instead.";
+ } else if (auto* vec = arr->ElemType()->As<sem::Vector>();
+ vec && vec->type()->Size() == 4) {
+ hint = "Consider using a vec4 instead.";
+ } else if (arr->ElemType()->Is<sem::Struct>()) {
+ hint =
+ "Consider using the @size attribute on the last struct "
+ "member.";
+ } else {
+ hint =
+ "Consider wrapping the element type in a struct and using "
+ "the "
+ "@size attribute.";
}
+ AddError(
+ "uniform storage requires that array elements be aligned to 16 "
+ "bytes, but array element alignment is currently " +
+ std::to_string(arr->Stride()) + ". " + hint,
+ source);
+ return false;
}
}
}
@@ -368,10 +384,20 @@
bool Resolver::ValidateStorageClassLayout(const sem::Variable* var) {
if (auto* str = var->Type()->UnwrapRef()->As<sem::Struct>()) {
- if (!ValidateStorageClassLayout(str, var->StorageClass())) {
+ if (!ValidateStorageClassLayout(str, var->StorageClass(),
+ str->Declaration()->source)) {
AddNote("see declaration of variable", var->Declaration()->source);
return false;
}
+ } else {
+ Source source = var->Declaration()->source;
+ if (var->Declaration()->type) {
+ source = var->Declaration()->type->source;
+ }
+ if (!ValidateStorageClassLayout(var->Type()->UnwrapRef(),
+ var->StorageClass(), source)) {
+ return false;
+ }
}
return true;
diff --git a/src/resolver/storage_class_layout_validation_test.cc b/src/resolver/storage_class_layout_validation_test.cc
index a8e7056..9355082 100644
--- a/src/resolver/storage_class_layout_validation_test.cc
+++ b/src/resolver/storage_class_layout_validation_test.cc
@@ -405,7 +405,7 @@
ASSERT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
- R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment of 'inner' is currently 4. Consider using a vector or struct as the element type instead.
+ R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.
12:34 note: see layout of struct:
/* align(4) size(44) */ struct Outer {
/* offset( 0) align(4) size(40) */ inner : array<f32, 10>;
@@ -442,7 +442,7 @@
ASSERT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
- R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment of 'inner' is currently 8. Consider using a vec4 instead.
+ R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 8. Consider using a vec4 instead.
12:34 note: see layout of struct:
/* align(8) size(88) */ struct Outer {
/* offset( 0) align(8) size(80) */ inner : array<vec2<f32>, 10>;
@@ -488,7 +488,7 @@
ASSERT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
- R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment of 'inner' is currently 8. Consider using the @size attribute on the last struct member.
+ R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 8. Consider using the @size attribute on the last struct member.
12:34 note: see layout of struct:
/* align(4) size(84) */ struct Outer {
/* offset( 0) align(4) size(80) */ inner : array<ArrayElem, 10>;
@@ -498,6 +498,49 @@
}
TEST_F(ResolverStorageClassLayoutValidationTest,
+ UniformBuffer_InvalidArrayStride_TopLevelArray) {
+ // @group(0) @binding(0)
+ // var<uniform> a : array<f32, 4>;
+ Global(Source{{78, 90}}, "a", ty.array(Source{{34, 56}}, ty.f32(), 4),
+ ast::StorageClass::kUniform, GroupAndBinding(0, 0));
+
+ ASSERT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.)");
+}
+
+TEST_F(ResolverStorageClassLayoutValidationTest,
+ UniformBuffer_InvalidArrayStride_NestedArray) {
+ // struct Outer {
+ // inner : array<array<f32, 4>, 4>
+ // };
+ //
+ // @group(0) @binding(0)
+ // var<uniform> a : array<Outer, 4>;
+
+ Structure(
+ Source{{12, 34}}, "Outer",
+ {
+ Member("inner", ty.array(Source{{34, 56}}, ty.array(ty.f32(), 4), 4)),
+ },
+ {StructBlock()});
+
+ Global(Source{{78, 90}}, "a", ty.type_name("Outer"),
+ ast::StorageClass::kUniform, GroupAndBinding(0, 0));
+
+ ASSERT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.
+12:34 note: see layout of struct:
+/* align(4) size(64) */ struct Outer {
+/* offset( 0) align(4) size(64) */ inner : array<array<f32, 4>, 4>;
+/* */ };
+78:90 note: see declaration of variable)");
+}
+
+TEST_F(ResolverStorageClassLayoutValidationTest,
UniformBuffer_InvalidArrayStride_SuggestedFix) {
// type Inner = @stride(16) array<f32, 10>;
//
diff --git a/src/resolver/storage_class_validation_test.cc b/src/resolver/storage_class_validation_test.cc
index 9965e08..63fdcac 100644
--- a/src/resolver/storage_class_validation_test.cc
+++ b/src/resolver/storage_class_validation_test.cc
@@ -302,8 +302,11 @@
}
TEST_F(ResolverStorageClassValidationTest, UniformBufferArray) {
+ // struct S {
+ // @size(16) f : f32;
+ // }
// var<uniform> g : array<S, 3>;
- auto* s = Structure("S", {Member("a", ty.f32())});
+ auto* s = Structure("S", {Member("a", ty.f32(), {MemberSize(16)})});
auto* a = ty.array(ty.Of(s), 3);
Global(Source{{56, 78}}, "g", a, ast::StorageClass::kUniform,
ast::DecorationList{
