writer/wgsl: Fix size / align decoration emission
This was broken by a rebase of the Default Struct Layout change.
This went unnoticed because there was no test coverage for these. Added.
Also replace `[[offset(n)]]` decorations with padding fields.
Bug: tint:626
Change-Id: Iad6f1a239bc8d8fcb15d18a204d3f5a78a372350
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/44683
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: David Neto <dneto@google.com>
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index a8fdecd..405a80b 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -1188,7 +1188,7 @@
offset = utils::RoundUp(align, offset);
auto* sem_member =
- builder_->create<semantic::StructMember>(member, offset, size);
+ builder_->create<semantic::StructMember>(member, offset, align, size);
builder_->Sem().Add(member, sem_member);
sem_members.emplace_back(sem_member);
diff --git a/src/semantic/sem_struct.cc b/src/semantic/sem_struct.cc
index 0d3b971..7050385 100644
--- a/src/semantic/sem_struct.cc
+++ b/src/semantic/sem_struct.cc
@@ -30,8 +30,9 @@
StructMember::StructMember(ast::StructMember* declaration,
uint32_t offset,
+ uint32_t align,
uint32_t size)
- : declaration_(declaration), offset_(offset), size_(size) {}
+ : declaration_(declaration), offset_(offset), align_(align), size_(size) {}
StructMember::~StructMember() = default;
diff --git a/src/semantic/struct.h b/src/semantic/struct.h
index 3d99655..0a80a0e 100644
--- a/src/semantic/struct.h
+++ b/src/semantic/struct.h
@@ -85,8 +85,12 @@
/// Constructor
/// @param declaration the AST declaration node
/// @param offset the byte offset from the base of the structure
- /// @param size the byte size
- StructMember(ast::StructMember* declaration, uint32_t offset, uint32_t size);
+ /// @param align the byte alignment of the member
+ /// @param size the byte size of the member
+ StructMember(ast::StructMember* declaration,
+ uint32_t offset,
+ uint32_t align,
+ uint32_t size);
/// Destructor
~StructMember() override;
@@ -97,13 +101,17 @@
/// @returns byte offset from base of structure
uint32_t Offset() const { return offset_; }
+ /// @returns the alignment of the member in bytes
+ uint32_t Align() const { return align_; }
+
/// @returns byte size
uint32_t Size() const { return size_; }
private:
ast::StructMember* const declaration_;
uint32_t const offset_; // Byte offset from base of structure
- uint32_t const size_; // Byte size
+ uint32_t const align_; // Byte alignment of the member
+ uint32_t const size_; // Byte size of the member
};
} // namespace semantic
diff --git a/src/writer/wgsl/generator_impl.cc b/src/writer/wgsl/generator_impl.cc
index 814dbf2..6539d93 100644
--- a/src/writer/wgsl/generator_impl.cc
+++ b/src/writer/wgsl/generator_impl.cc
@@ -15,6 +15,7 @@
#include "src/writer/wgsl/generator_impl.h"
#include <algorithm>
+#include <limits>
#include "src/ast/bool_literal.h"
#include "src/ast/call_statement.h"
@@ -31,6 +32,7 @@
#include "src/ast/variable_decl_statement.h"
#include "src/ast/workgroup_decoration.h"
#include "src/semantic/function.h"
+#include "src/semantic/struct.h"
#include "src/semantic/variable.h"
#include "src/type/access_control_type.h"
#include "src/type/alias_type.h"
@@ -46,6 +48,7 @@
#include "src/type/u32_type.h"
#include "src/type/vector_type.h"
#include "src/type/void_type.h"
+#include "src/utils/math.h"
#include "src/writer/float_to_string.h"
namespace tint {
@@ -503,11 +506,41 @@
out_ << "struct " << program_->Symbols().NameFor(str->symbol()) << " {"
<< std::endl;
+ auto add_padding = [&](uint32_t size) {
+ make_indent();
+ out_ << "[[size(" << size << ")]]" << std::endl;
+ make_indent();
+ // Note: u32 is the smallest primitive we currently support. When WGSL
+ // supports smaller types, this will need to be updated.
+ out_ << UniqueIdentifier("padding") << " : u32;" << std::endl;
+ };
+
increment_indent();
+ uint32_t offset = 0;
for (auto* mem : impl->members()) {
- if (!mem->decorations().empty()) {
+ auto* mem_sem = program_->Sem().Get(mem);
+
+ offset = utils::RoundUp(mem_sem->Align(), offset);
+ if (uint32_t padding = mem_sem->Offset() - offset) {
+ add_padding(padding);
+ offset += padding;
+ }
+ offset += mem_sem->Size();
+
+ // Offset decorations no longer exist in the WGSL spec, but are emitted
+ // by the SPIR-V reader and are consumed by the Resolver(). These should not
+ // be emitted, but instead struct padding fields should be emitted.
+ ast::DecorationList decorations_sanitized;
+ decorations_sanitized.reserve(mem->decorations().size());
+ for (auto* deco : mem->decorations()) {
+ if (!deco->Is<ast::StructMemberOffsetDecoration>()) {
+ decorations_sanitized.emplace_back(deco);
+ }
+ }
+
+ if (!decorations_sanitized.empty()) {
make_indent();
- if (!EmitDecorations(mem->decorations())) {
+ if (!EmitDecorations(decorations_sanitized)) {
return false;
}
out_ << std::endl;
@@ -585,14 +618,13 @@
out_ << "builtin(" << builtin->value() << ")";
} else if (auto* constant = deco->As<ast::ConstantIdDecoration>()) {
out_ << "constant_id(" << constant->value() << ")";
- } else if (auto* offset = deco->As<ast::StructMemberOffsetDecoration>()) {
- out_ << "offset(" << offset->offset() << ")";
} else if (auto* size = deco->As<ast::StructMemberSizeDecoration>()) {
- out_ << "[[size(" << size->size() << ")]]" << std::endl;
+ out_ << "size(" << size->size() << ")";
} else if (auto* align = deco->As<ast::StructMemberAlignDecoration>()) {
- out_ << "[[align(" << align->align() << ")]]" << std::endl;
+ out_ << "align(" << align->align() << ")";
} else {
- diagnostics_.add_error("unknown variable decoration");
+ TINT_ICE(diagnostics_)
+ << "Unsupported decoration '" << deco->TypeInfo().name << "'";
return false;
}
}
@@ -952,6 +984,23 @@
return true;
}
+std::string GeneratorImpl::UniqueIdentifier(const std::string& suffix) {
+ auto const limit =
+ std::numeric_limits<decltype(next_unique_identifier_suffix)>::max();
+ while (next_unique_identifier_suffix < limit) {
+ auto ident = "tint_" + std::to_string(next_unique_identifier_suffix);
+ if (!suffix.empty()) {
+ ident += "_" + suffix;
+ }
+ next_unique_identifier_suffix++;
+ if (!program_->Symbols().Get(ident).IsValid()) {
+ return ident;
+ }
+ }
+ diagnostics_.add_error("Unable to generate a unique WGSL identifier");
+ return "<invalid-ident>";
+}
+
} // namespace wgsl
} // namespace writer
} // namespace tint
diff --git a/src/writer/wgsl/generator_impl.h b/src/writer/wgsl/generator_impl.h
index b5b50a6..785c4cb 100644
--- a/src/writer/wgsl/generator_impl.h
+++ b/src/writer/wgsl/generator_impl.h
@@ -199,7 +199,11 @@
bool EmitDecorations(const ast::DecorationList& decos);
private:
+ /// @return a new, unique, valid WGSL identifier with the given suffix.
+ std::string UniqueIdentifier(const std::string& suffix = "");
+
Program const* const program_;
+ uint32_t next_unique_identifier_suffix = 0;
};
} // namespace wgsl
diff --git a/src/writer/wgsl/generator_impl_type_test.cc b/src/writer/wgsl/generator_impl_type_test.cc
index 3d4d2ca..c336bac 100644
--- a/src/writer/wgsl/generator_impl_type_test.cc
+++ b/src/writer/wgsl/generator_impl_type_test.cc
@@ -159,33 +159,90 @@
EXPECT_EQ(gen.result(), "S");
}
-TEST_F(WgslGeneratorImplTest, EmitType_StructDecl) {
+TEST_F(WgslGeneratorImplTest, EmitType_StructOffsetDecl) {
auto* s = Structure("S", {
- Member("a", ty.i32()),
- Member("b", ty.f32(), {MemberOffset(4)}),
+ Member("a", ty.i32(), {MemberOffset(8)}),
+ Member("b", ty.f32(), {MemberOffset(16)}),
});
GeneratorImpl& gen = Build();
ASSERT_TRUE(gen.EmitStructType(s)) << gen.error();
EXPECT_EQ(gen.result(), R"(struct S {
+ [[size(8)]]
+ tint_0_padding : u32;
a : i32;
- [[offset(4)]]
+ [[size(4)]]
+ tint_1_padding : u32;
+ b : f32;
+};
+)");
+}
+
+TEST_F(WgslGeneratorImplTest, EmitType_StructOffsetDecl_WithSymbolCollisions) {
+ auto* s =
+ Structure("S", {
+ Member("tint_0_padding", ty.i32(), {MemberOffset(8)}),
+ Member("tint_2_padding", ty.f32(), {MemberOffset(16)}),
+ });
+
+ GeneratorImpl& gen = Build();
+
+ ASSERT_TRUE(gen.EmitStructType(s)) << gen.error();
+ EXPECT_EQ(gen.result(), R"(struct S {
+ [[size(8)]]
+ tint_1_padding : u32;
+ tint_0_padding : i32;
+ [[size(4)]]
+ tint_3_padding : u32;
+ tint_2_padding : f32;
+};
+)");
+}
+
+TEST_F(WgslGeneratorImplTest, EmitType_StructAlignDecl) {
+ auto* s = Structure("S", {
+ Member("a", ty.i32(), {MemberAlign(8)}),
+ Member("b", ty.f32(), {MemberAlign(16)}),
+ });
+
+ GeneratorImpl& gen = Build();
+
+ ASSERT_TRUE(gen.EmitStructType(s)) << gen.error();
+ EXPECT_EQ(gen.result(), R"(struct S {
+ [[align(8)]]
+ a : i32;
+ [[align(16)]]
+ b : f32;
+};
+)");
+}
+
+TEST_F(WgslGeneratorImplTest, EmitType_StructSizeDecl) {
+ auto* s = Structure("S", {
+ Member("a", ty.i32(), {MemberSize(16)}),
+ Member("b", ty.f32(), {MemberSize(32)}),
+ });
+
+ GeneratorImpl& gen = Build();
+
+ ASSERT_TRUE(gen.EmitStructType(s)) << gen.error();
+ EXPECT_EQ(gen.result(), R"(struct S {
+ [[size(16)]]
+ a : i32;
+ [[size(32)]]
b : f32;
};
)");
}
TEST_F(WgslGeneratorImplTest, EmitType_Struct_WithDecoration) {
- ast::DecorationList decos;
- decos.push_back(create<ast::StructBlockDecoration>());
-
auto* s = Structure("S",
{
Member("a", ty.i32()),
- Member("b", ty.f32(), {MemberOffset(4)}),
+ Member("b", ty.f32(), {MemberAlign(8)}),
},
- decos);
+ {create<ast::StructBlockDecoration>()});
GeneratorImpl& gen = Build();
@@ -193,7 +250,7 @@
EXPECT_EQ(gen.result(), R"([[block]]
struct S {
a : i32;
- [[offset(4)]]
+ [[align(8)]]
b : f32;
};
)");
