Skip to content

Commit

Permalink
[ir] Use inference for nested arrays of builtin structures
Browse files Browse the repository at this point in the history
When emitting array types, check for builtin structures at any nested
depth to determine whether the type can be spelled out explicitly or
whether inference has to be used for the top-level array.

Fixed: 380898799
Change-Id: I672f21a4cfad8ddd8ab9d6199b7ddfd44e0e9a73
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/217994
Reviewed-by: dan sinclair <[email protected]>
Commit-Queue: James Price <[email protected]>
  • Loading branch information
jrprice authored and Dawn LUCI CQ committed Dec 13, 2024
1 parent 737d588 commit bfcce9f
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 4 deletions.
10 changes: 10 additions & 0 deletions src/tint/lang/wgsl/ir_roundtrip_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3401,6 +3401,16 @@ fn a(x : f32) {
)");
}

// Test that we do not try to name the unnameable builtin structure types in nested array
// declarations. See crbug.com/380898799.
TEST_F(IRToProgramRoundtripTest, BuiltinStructInInferredNestedArrayType) {
RUN_TEST(R"(
fn a(x : f32) {
let y = array(array(frexp(x)));
}
)");
}

// Test that we rename declarations that shadow builtin types when they are used in arrays.
// See crbug.com/380903161.
TEST_F(IRToProgramRoundtripTest, BuiltinTypeNameShadowedAndUsedInArray) {
Expand Down
21 changes: 17 additions & 4 deletions src/tint/lang/wgsl/writer/ir_to_program/ir_to_program.cc
Original file line number Diff line number Diff line change
Expand Up @@ -988,12 +988,12 @@ class State {
}
},
[&](const core::type::Array* a) {
auto el = Type(a->ElemType());
if (!el) {
if (ContainsBuiltinStruct(a)) {
// The element type is untypeable, so we need to infer it instead.
return ast::Type{b.Expr(b.Ident("array"))};
}

auto el = Type(a->ElemType());
Vector<const ast::Attribute*, 1> attrs;
if (!a->IsStrideImplicit()) {
attrs.Push(b.Stride(a->Stride()));
Expand Down Expand Up @@ -1053,8 +1053,7 @@ class State {

ast::Type Struct(const core::type::Struct* s) {
// Skip builtin structures.
// TODO(350778507): Consider using a struct flag for builtin structures instead.
if (tint::HasPrefix(s->Name().NameView(), "__")) {
if (ContainsBuiltinStruct(s)) {
return ast::Type{};
}

Expand Down Expand Up @@ -1108,6 +1107,20 @@ class State {
return b.ty(n);
}

bool ContainsBuiltinStruct(const core::type::Type* ty) {
if (auto* s = ty->As<core::type::Struct>()) {
// Note: We don't need to check the members of the struct, as builtin structures cannot
// be nested inside other structures.
// TODO(350778507): Consider using a struct flag for builtin structures instead.
if (tint::HasPrefix(s->Name().NameView(), "__")) {
return true;
}
} else if (auto* a = ty->As<core::type::Array>()) {
return ContainsBuiltinStruct(a->ElemType());
}
return false;
}

////////////////////////////////////////////////////////////////////////////////////////////////
// Bindings
////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down

0 comments on commit bfcce9f

Please sign in to comment.