[tint][val] Check that RTAs are only the last member of structs Includes some rewriting of the struct type check to avoid accidentally skipping unrelated checks when matrix decorations are allowed. Fixes: 445906424 Change-Id: I57c73aa6580e9d08411f12be7fe7d305c5a7eb74 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/264554 Commit-Queue: dan sinclair <dsinclair@chromium.org> Commit-Queue: James Price <jrprice@google.com> Reviewed-by: dan sinclair <dsinclair@chromium.org> Auto-Submit: Ryan Harrison <rharrison@chromium.org> Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/tint/lang/core/ir/validator.cc b/src/tint/lang/core/ir/validator.cc index 60f586b..1661630 100644 --- a/src/tint/lang/core/ir/validator.cc +++ b/src/tint/lang/core/ir/validator.cc
@@ -2018,32 +2018,23 @@ return tint::Switch( type, [&](const core::type::Struct* str) { - if (capabilities_.Contains(Capability::kAllowStructMatrixDecorations)) { - return true; - } - for (auto* member : str->Members()) { CheckType(member->Type(), diag, ignore_caps); - if (member->RowMajor()) { - diag() << "Row major annotation not allowed on structures"; - return false; - } - if (member->HasMatrixStride()) { - diag() << "Matrix stride annotation not allowed on structures"; - return false; - } - if (member->Size() < member->Type()->Size()) { - diag() << "struct member " << member->Index() - << " with size=" << member->Size() - << " must be at least as large as the type with size " - << member->Type()->Size(); - return false; - } if (member->Type()->Is<core::type::Void>()) { diag() << "struct member " << member->Index() << " cannot have void type"; return false; } + + if (auto* arr = member->Type()->As<core::type::Array>(); + arr && arr->Count()->Is<core::type::RuntimeArrayCount>()) { + if (member != str->Members().Back()) { + diag() << "runtime-sized arrays can only be the last member of a " + "struct"; + return false; + } + } + if (member->Align() == 0) { diag() << "struct member must not have an alignment of 0"; return false; @@ -2052,11 +2043,34 @@ diag() << "struct member type must not have an alignment of 0"; return false; } - if (member->Align() % member->Type()->Align() != 0) { - diag() << "struct member alignment (" << member->Align() - << ") must be divisible by type alignment (" - << member->Type()->Align() << ")"; - return false; + + if (!capabilities_.Contains(Capability::kAllowStructMatrixDecorations)) { + if (member->RowMajor()) { + diag() << "Row major annotation not allowed on structures"; + return false; + } + if (member->HasMatrixStride()) { + diag() << "Matrix stride annotation not allowed on structures"; + return false; + } + } + + // TODO(448608979): Remove guard once updated to handle RowMajor correctly + if (!member->RowMajor()) { + if (member->Size() < member->Type()->Size()) { + diag() << "struct member " << member->Index() + << " with size=" << member->Size() + << " must be at least as large as the type with size " + << member->Type()->Size(); + return false; + } + + if (member->Align() % member->Type()->Align() != 0) { + diag() << "struct member alignment (" << member->Align() + << ") must be divisible by type alignment (" + << member->Type()->Align() << ")"; + return false; + } } } return true;
diff --git a/src/tint/lang/core/ir/validator_type_test.cc b/src/tint/lang/core/ir/validator_type_test.cc index a67dbc9..c8564b2 100644 --- a/src/tint/lang/core/ir/validator_type_test.cc +++ b/src/tint/lang/core/ir/validator_type_test.cc
@@ -314,6 +314,164 @@ )")) << res.Failure(); } +TEST_F(IR_ValidatorTest, StructureMember_SizeTooSmall) { + auto* str_ty = + ty.Struct(mod.symbols.New("S"), + Vector{ + ty.Get<type::StructMember>(mod.symbols.New("a"), ty.array<u32, 3>(), 0u, 0u, + 4u, 4u, IOAttributes{}), + }); + mod.root_block->Append(b.Var("my_struct", private_, str_ty)); + + auto* fn = b.Function("F", ty.void_()); + b.Append(fn->Block(), [&] { b.Return(fn); }); + + auto res = ir::Validate(mod); + ASSERT_NE(res, Success); + EXPECT_THAT( + res.Failure().reason, + testing::HasSubstr( + "struct member 0 with size=4 must be at least as large as the type with size 12")) + << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_RuntimeArrayNotLast) { + auto* s1 = ty.Struct(mod.symbols.New("S1"), {{mod.symbols.New("a"), ty.u32()}}); + auto* rta = ty.runtime_array(s1); + + auto* str_ty = ty.Struct(mod.symbols.New("OuterS"), { + {mod.symbols.New("a1"), rta}, + {mod.symbols.New("j"), ty.u32()}, + }); + + auto* v = b.Var(ty.ptr(storage, str_ty, read_write)); + v->SetBindingPoint(0, 0); + mod.root_block->Append(v); + + auto res = ir::Validate(mod); + ASSERT_NE(res, Success); + EXPECT_THAT( + res.Failure().reason, + testing::HasSubstr( + R"(:11:3 error: var: runtime-sized arrays can only be the last member of a struct + %1:ptr<storage, OuterS, read_write> = var undef @binding_point(0, 0) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^)")) + << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_RuntimeArrayIsLast) { + auto* s1 = ty.Struct(mod.symbols.New("S1"), {{mod.symbols.New("a"), ty.u32()}}); + auto* rta = ty.runtime_array(s1); + + auto* str_ty = ty.Struct(mod.symbols.New("OuterS"), { + {mod.symbols.New("j"), ty.u32()}, + {mod.symbols.New("a1"), rta}, + }); + + auto* v = b.Var(ty.ptr(storage, str_ty, read_write)); + v->SetBindingPoint(0, 0); + mod.root_block->Append(v); + + auto res = ir::Validate(mod); + ASSERT_EQ(res, Success) << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_MultipleRuntimeArrays) { + auto* s1 = ty.Struct(mod.symbols.New("S1"), {{mod.symbols.New("a"), ty.u32()}}); + auto* rta = ty.runtime_array(s1); + + auto* str_ty = ty.Struct(mod.symbols.New("OuterS"), { + {mod.symbols.New("a1"), rta}, + {mod.symbols.New("a2"), rta}, + }); + + auto* v = b.Var(ty.ptr(storage, str_ty, read_write)); + v->SetBindingPoint(0, 0); + mod.root_block->Append(v); + + auto res = ir::Validate(mod); + ASSERT_NE(res, Success); + EXPECT_THAT( + res.Failure().reason, + testing::HasSubstr( + R"(:11:3 error: var: runtime-sized arrays can only be the last member of a struct + %1:ptr<storage, OuterS, read_write> = var undef @binding_point(0, 0) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^)")) + << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_RowMajor_WithoutCapability) { + auto* mat_ty = ty.mat2x2<f32>(); + auto* member = ty.Get<core::type::StructMember>( + mod.symbols.New("m"), mat_ty, 0u, 0u, mat_ty->Align(), mat_ty->Size(), IOAttributes{}); + member->SetRowMajor(); + auto* str_ty = + ty.Get<core::type::Struct>(mod.symbols.New("MyStruct"), Vector{member}, mat_ty->Size()); + + auto* v = b.Var(ty.ptr(private_, str_ty)); + mod.root_block->Append(v); + + auto res = ir::Validate(mod); + ASSERT_NE(res, Success); + EXPECT_THAT( + res.Failure().reason, + testing::HasSubstr(R"(:6:3 error: var: Row major annotation not allowed on structures + %1:ptr<private, MyStruct, read_write> = var undef + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +)")) << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_RowMajor_WithCapability) { + auto* mat_ty = ty.mat2x2<f32>(); + auto* member = ty.Get<core::type::StructMember>( + mod.symbols.New("m"), mat_ty, 0u, 0u, mat_ty->Align(), mat_ty->Size(), IOAttributes{}); + member->SetRowMajor(); + auto* str_ty = + ty.Get<core::type::Struct>(mod.symbols.New("MyStruct"), Vector{member}, mat_ty->Size()); + + auto* v = b.Var(ty.ptr(private_, str_ty)); + mod.root_block->Append(v); + + auto res = ir::Validate(mod, Capabilities{Capability::kAllowStructMatrixDecorations}); + ASSERT_EQ(res, Success) << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_MatrixStride_WithoutCapability) { + auto* mat_ty = ty.mat2x2<f32>(); + auto* member = ty.Get<core::type::StructMember>( + mod.symbols.New("m"), mat_ty, 0u, 0u, mat_ty->Align(), mat_ty->Size(), IOAttributes{}); + member->SetMatrixStride(32); + auto* str_ty = + ty.Get<core::type::Struct>(mod.symbols.New("MyStruct"), Vector{member}, mat_ty->Size()); + + auto* v = b.Var(ty.ptr(private_, str_ty)); + mod.root_block->Append(v); + + auto res = ir::Validate(mod); + ASSERT_NE(res, Success); + EXPECT_THAT( + res.Failure().reason, + testing::HasSubstr(R"(:6:3 error: var: Matrix stride annotation not allowed on structures + %1:ptr<private, MyStruct, read_write> = var undef + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +)")) << res.Failure(); +} + +TEST_F(IR_ValidatorTest, StructMember_MatrixStride_WithCapability) { + auto* mat_ty = ty.mat2x2<f32>(); + auto* member = ty.Get<core::type::StructMember>( + mod.symbols.New("m"), mat_ty, 0u, 0u, mat_ty->Align(), mat_ty->Size(), IOAttributes{}); + member->SetMatrixStride(32); + auto* str_ty = + ty.Get<core::type::Struct>(mod.symbols.New("MyStruct"), Vector{member}, mat_ty->Size()); + + auto* v = b.Var(ty.ptr(private_, str_ty)); + mod.root_block->Append(v); + + auto res = ir::Validate(mod, Capabilities{Capability::kAllowStructMatrixDecorations}); + ASSERT_EQ(res, Success) << res.Failure(); +} + TEST_F(IR_ValidatorTest, FunctionParam_InvalidAddressSpaceForHandleType) { auto* type = ty.ptr(AddressSpace::kFunction, ty.sampler()); auto* fn = b.Function("my_func", ty.void_()); @@ -1342,27 +1500,6 @@ } } -TEST_F(IR_ValidatorTest, StructureMemberSizeTooSmall) { - auto* str_ty = - ty.Struct(mod.symbols.New("S"), - Vector{ - ty.Get<type::StructMember>(mod.symbols.New("a"), ty.array<u32, 3>(), 0u, 0u, - 4u, 4u, IOAttributes{}), - }); - mod.root_block->Append(b.Var("my_struct", private_, str_ty)); - - auto* fn = b.Function("F", ty.void_()); - b.Append(fn->Block(), [&] { b.Return(fn); }); - - auto res = ir::Validate(mod); - ASSERT_NE(res, Success); - EXPECT_THAT( - res.Failure().reason, - testing::HasSubstr( - "struct member 0 with size=4 must be at least as large as the type with size 12")) - << res.Failure(); -} - INSTANTIATE_TEST_SUITE_P(IR_ValidatorTest, AddressSpace_AccessMode, testing::Combine(testing::Values(AddressSpace::kFunction,