encounter
/
dawn-cmake
mirror of https://github.com/encounter/dawn-cmake.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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>
This commit is contained in:
James Price 2022-01-20 22:11:07 +00:00
parent f6e5cc03bf
commit 533a1bae68
4 changed files with 165 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/resolver/resolver.h
View File

@ -108,8 +108,8 @@ class Resolver {
/// 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 @@ class Resolver {
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

194
src/resolver/resolver_validation.cc
View File

@ -229,8 +229,9 @@ bool Resolver::ValidateVariableConstructorOrCast(
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 @@ bool Resolver::ValidateStorageClassLayout(const sem::Struct* str,
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::Struct* str,
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;

49
src/resolver/storage_class_layout_validation_test.cc
View File

@ -405,7 +405,7 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
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 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
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 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
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>;
@ -497,6 +497,49 @@ TEST_F(ResolverStorageClassLayoutValidationTest,
78:90 note: see declaration of variable)");
}
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>;

5
src/resolver/storage_class_validation_test.cc
View File

@ -302,8 +302,11 @@ TEST_F(ResolverStorageClassValidationTest, UniformBufferVector) {
}
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{