From 885488da41a0ea4e08f2221f2fb01c66498338be Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Wed, 30 Jun 2021 16:01:40 +0000 Subject: [PATCH] writer/msl: Use UniqueIdentifier() for padding field names UniqueIdentifier() will generate a program-global unique symbol. MslGeneratorImplTest.AttemptTintPadSymbolCollision tests for collisions with the field names. TextGeneratorTest.UniqueIdentifier_ConflictWithExisting tests for collisions between general symbols. Fixed: tint:654 Change-Id: If2ba75d04ff0e2a9975e878596ac114d51adcd46 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/56580 Kokoro: Kokoro Reviewed-by: Sarah Mashayekhi Reviewed-by: James Price Commit-Queue: James Price Auto-Submit: Ben Clayton --- src/writer/msl/generator_impl.cc | 3 +- src/writer/msl/generator_impl_type_test.cc | 36 +++++++++---------- .../assign_to_function_var.wgsl.expected.msl | 2 +- .../assign_to_private_var.wgsl.expected.msl | 2 +- .../assign_to_storage_var.wgsl.expected.msl | 2 +- .../assign_to_workgroup_var.wgsl.expected.msl | 2 +- test/bug/tint/294.wgsl.expected.msl | 2 +- ...ed_struct_storage_buffer.wgsl.expected.msl | 2 +- 8 files changed, 25 insertions(+), 26 deletions(-) diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc index bf7ffabc7c..2920e80cdb 100644 --- a/src/writer/msl/generator_impl.cc +++ b/src/writer/msl/generator_impl.cc @@ -1885,11 +1885,10 @@ bool GeneratorImpl::EmitStructType(const sem::Struct* str) { out.flags(saved_flag_state); }; - uint32_t pad_count = 0; auto add_padding = [&](uint32_t size, uint32_t msl_offset) { std::string name; do { - name = "tint_pad_" + std::to_string(pad_count++); + name = UniqueIdentifier("tint_pad"); } while (str->FindMember(program_->Symbols().Get(name))); auto out = line(); diff --git a/src/writer/msl/generator_impl_type_test.cc b/src/writer/msl/generator_impl_type_test.cc index d595724a89..fa55b75814 100644 --- a/src/writer/msl/generator_impl_type_test.cc +++ b/src/writer/msl/generator_impl_type_test.cc @@ -133,7 +133,7 @@ TEST_F(MslGeneratorImplTest, EmitType_ArrayWithStride) { ASSERT_TRUE(gen.Generate()) << gen.error(); EXPECT_THAT(gen.result(), HasSubstr(R"(struct tint_padded_array_element { /* 0x0000 */ float el; - /* 0x0004 */ int8_t tint_pad_0[60]; + /* 0x0004 */ int8_t tint_pad[60]; };)")); EXPECT_THAT(gen.result(), HasSubstr(R"(struct tint_array_wrapper { /* 0x0000 */ tint_padded_array_element arr[4]; @@ -275,7 +275,7 @@ TEST_F(MslGeneratorImplTest, EmitType_Struct_Layout_NonComposites) { // for each field of the structure s. #define ALL_FIELDS() \ FIELD(0x0000, int, a, /*NO SUFFIX*/) \ - FIELD(0x0004, int8_t, tint_pad_0, [124]) \ + FIELD(0x0004, int8_t, tint_pad, [124]) \ FIELD(0x0080, float, b, /*NO SUFFIX*/) \ FIELD(0x0084, int8_t, tint_pad_1, [124]) \ FIELD(0x0100, packed_float2, c, /*NO SUFFIX*/) \ @@ -384,7 +384,7 @@ TEST_F(MslGeneratorImplTest, EmitType_Struct_Layout_Structures) { // for each field of the structure s. #define ALL_FIELDS() \ FIELD(0x0000, int, a, /*NO SUFFIX*/) \ - FIELD(0x0004, int8_t, tint_pad_0, [508]) \ + FIELD(0x0004, int8_t, tint_pad, [508]) \ FIELD(0x0200, inner_x, b, /*NO SUFFIX*/) \ FIELD(0x0600, float, c, /*NO SUFFIX*/) \ FIELD(0x0604, inner_y, d, /*NO SUFFIX*/) \ @@ -476,14 +476,14 @@ TEST_F(MslGeneratorImplTest, EmitType_Struct_Layout_ArrayDefaultStride) { // ALL_FIELDS() calls the macro FIELD(ADDR, TYPE, NAME, SUFFIX) // for each field of the structure s. -#define ALL_FIELDS() \ - FIELD(0x0000, int, a, /*NO SUFFIX*/) \ - FIELD(0x0004, float, b, [7]) \ - FIELD(0x0020, float, c, /*NO SUFFIX*/) \ - FIELD(0x0024, int8_t, tint_pad_0, [476]) \ - FIELD(0x0200, inner, d, [4]) \ - FIELD(0x1200, float, e, /*NO SUFFIX*/) \ - FIELD(0x1204, float, f, [1]) \ +#define ALL_FIELDS() \ + FIELD(0x0000, int, a, /*NO SUFFIX*/) \ + FIELD(0x0004, float, b, [7]) \ + FIELD(0x0020, float, c, /*NO SUFFIX*/) \ + FIELD(0x0024, int8_t, tint_pad, [476]) \ + FIELD(0x0200, inner, d, [4]) \ + FIELD(0x1200, float, e, /*NO SUFFIX*/) \ + FIELD(0x1204, float, f, [1]) \ FIELD(0x1208, int8_t, tint_pad_1, [504]) // Check that the generated string is as expected. @@ -561,11 +561,11 @@ TEST_F(MslGeneratorImplTest, EmitType_Struct_Layout_ArrayVec3DefaultStride) { // ALL_FIELDS() calls the macro FIELD(ADDR, TYPE, NAME, SUFFIX) // for each field of the structure s. -#define ALL_FIELDS() \ - FIELD(0x0000, int, a, /*NO SUFFIX*/) \ - FIELD(0x0004, int8_t, tint_pad_0, [12]) \ - FIELD(0x0010, float3, b, [4]) \ - FIELD(0x0050, int, c, /*NO SUFFIX*/) \ +#define ALL_FIELDS() \ + FIELD(0x0000, int, a, /*NO SUFFIX*/) \ + FIELD(0x0004, int8_t, tint_pad, [12]) \ + FIELD(0x0010, float3, b, [4]) \ + FIELD(0x0050, int, c, /*NO SUFFIX*/) \ FIELD(0x0054, int8_t, tint_pad_1, [12]) // Check that the generated string is as expected. @@ -592,7 +592,7 @@ TEST_F(MslGeneratorImplTest, AttemptTintPadSymbolCollision) { Member("tint_pad_27", ty.mat2x2()), Member("tint_pad_24", ty.u32()), Member("tint_pad_23", ty.mat2x3()), - Member("tint_pad_0", ty.u32()), + Member("tint_pad", ty.u32()), Member("tint_pad_8", ty.mat2x4()), Member("tint_pad_26", ty.u32()), Member("tint_pad_29", ty.mat3x2()), @@ -637,7 +637,7 @@ TEST_F(MslGeneratorImplTest, AttemptTintPadSymbolCollision) { /* 0x0148 */ uint tint_pad_24; /* 0x014c */ int8_t tint_pad_14[4]; /* 0x0150 */ float2x3 tint_pad_23; - /* 0x0170 */ uint tint_pad_0; + /* 0x0170 */ uint tint_pad; /* 0x0174 */ int8_t tint_pad_15[12]; /* 0x0180 */ float2x4 tint_pad_8; /* 0x01a0 */ uint tint_pad_26; diff --git a/test/array/assign_to_function_var.wgsl.expected.msl b/test/array/assign_to_function_var.wgsl.expected.msl index 7dec2d0ba4..894f199fc7 100644 --- a/test/array/assign_to_function_var.wgsl.expected.msl +++ b/test/array/assign_to_function_var.wgsl.expected.msl @@ -3,7 +3,7 @@ using namespace metal; struct tint_padded_array_element { /* 0x0000 */ int el; - /* 0x0004 */ int8_t tint_pad_0[12]; + /* 0x0004 */ int8_t tint_pad[12]; }; struct tint_array_wrapper { /* 0x0000 */ tint_padded_array_element arr[4]; diff --git a/test/array/assign_to_private_var.wgsl.expected.msl b/test/array/assign_to_private_var.wgsl.expected.msl index 322db3b109..6542b6069c 100644 --- a/test/array/assign_to_private_var.wgsl.expected.msl +++ b/test/array/assign_to_private_var.wgsl.expected.msl @@ -3,7 +3,7 @@ using namespace metal; struct tint_padded_array_element { /* 0x0000 */ int el; - /* 0x0004 */ int8_t tint_pad_0[12]; + /* 0x0004 */ int8_t tint_pad[12]; }; struct tint_array_wrapper { /* 0x0000 */ tint_padded_array_element arr[4]; diff --git a/test/array/assign_to_storage_var.wgsl.expected.msl b/test/array/assign_to_storage_var.wgsl.expected.msl index f0a241a6fc..d1374b35fe 100644 --- a/test/array/assign_to_storage_var.wgsl.expected.msl +++ b/test/array/assign_to_storage_var.wgsl.expected.msl @@ -3,7 +3,7 @@ using namespace metal; struct tint_padded_array_element { /* 0x0000 */ int el; - /* 0x0004 */ int8_t tint_pad_0[12]; + /* 0x0004 */ int8_t tint_pad[12]; }; struct tint_array_wrapper { /* 0x0000 */ tint_padded_array_element arr[4]; diff --git a/test/array/assign_to_workgroup_var.wgsl.expected.msl b/test/array/assign_to_workgroup_var.wgsl.expected.msl index 242b669dbf..28d096569e 100644 --- a/test/array/assign_to_workgroup_var.wgsl.expected.msl +++ b/test/array/assign_to_workgroup_var.wgsl.expected.msl @@ -3,7 +3,7 @@ using namespace metal; struct tint_padded_array_element { /* 0x0000 */ int el; - /* 0x0004 */ int8_t tint_pad_0[12]; + /* 0x0004 */ int8_t tint_pad[12]; }; struct tint_array_wrapper { /* 0x0000 */ tint_padded_array_element arr[4]; diff --git a/test/bug/tint/294.wgsl.expected.msl b/test/bug/tint/294.wgsl.expected.msl index 224ceadfba..fb5e0c6f64 100644 --- a/test/bug/tint/294.wgsl.expected.msl +++ b/test/bug/tint/294.wgsl.expected.msl @@ -3,7 +3,7 @@ using namespace metal; struct Light { /* 0x0000 */ packed_float3 position; - /* 0x000c */ int8_t tint_pad_0[4]; + /* 0x000c */ int8_t tint_pad[4]; /* 0x0010 */ packed_float3 colour; /* 0x001c */ int8_t tint_pad_1[4]; }; diff --git a/test/shader_io/shared_struct_storage_buffer.wgsl.expected.msl b/test/shader_io/shared_struct_storage_buffer.wgsl.expected.msl index 63fe3383b7..17fece7588 100644 --- a/test/shader_io/shared_struct_storage_buffer.wgsl.expected.msl +++ b/test/shader_io/shared_struct_storage_buffer.wgsl.expected.msl @@ -4,7 +4,7 @@ using namespace metal; struct S { /* 0x0000 */ float f; /* 0x0004 */ uint u; - /* 0x0008 */ int8_t tint_pad_0[120]; + /* 0x0008 */ int8_t tint_pad[120]; /* 0x0080 */ packed_float4 v; /* 0x0090 */ int8_t tint_pad_1[112]; };