transform: Avoid symbol collision in Canonicalize IO

Correctly rename fields when combining two or more input structures together into a single input structure.

Bug: chromium:1251009
Change-Id: I0c7ab5ed3116b97035e100d1ef96e772e819f640
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/64545
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/transform/canonicalize_entry_point_io.cc

@@ -112,6 +112,8 @@ struct CanonicalizeEntryPointIO::State {
   std::vector<OutputValue> wrapper_output_values;
   /// The body of the wrapper function.
   ast::StatementList wrapper_body;
+  /// Input names used by the entrypoint
+  std::unordered_set<std::string> input_names;
 
   /// Constructor
   /// @param context the clone context
@@ -171,29 +173,35 @@ struct CanonicalizeEntryPointIO::State {
               ctx.dst->ID(), ast::DisabledValidation::kIgnoreStorageClass));
 
       // Create the global variable and use its value for the shader input.
-      auto var = ctx.dst->Symbols().New(name);
+      auto symbol = ctx.dst->Symbols().New(name);
-      ast::Expression* value = ctx.dst->Expr(var);
+      ast::Expression* value = ctx.dst->Expr(symbol);
       if (HasSampleMask(attributes)) {
         // Vulkan requires the type of a SampleMask builtin to be an array.
         // Declare it as array<u32, 1> and then load the first element.
         type = ctx.dst->ty.array(type, 1);
         value = ctx.dst->IndexAccessor(value, 0);
       }
-      ctx.dst->Global(var, type, ast::StorageClass::kInput,
+      ctx.dst->Global(symbol, type, ast::StorageClass::kInput,
                       std::move(attributes));
       return value;
     } else if (cfg.shader_style == ShaderStyle::kMsl &&
                ast::HasDecoration<ast::BuiltinDecoration>(attributes)) {
       // If this input is a builtin and we are targeting MSL, then add it to the
       // parameter list and pass it directly to the inner function.
+      Symbol symbol = input_names.emplace(name).second
+                          ? ctx.dst->Symbols().Register(name)
+                          : ctx.dst->Symbols().New(name);
       wrapper_ep_parameters.push_back(
-          ctx.dst->Param(name, type, std::move(attributes)));
+          ctx.dst->Param(symbol, type, std::move(attributes)));
-      return ctx.dst->Expr(name);
+      return ctx.dst->Expr(symbol);
     } else {
       // Otherwise, move it to the new structure member list.
+      Symbol symbol = input_names.emplace(name).second
+                          ? ctx.dst->Symbols().Register(name)
+                          : ctx.dst->Symbols().New(name);
       wrapper_struct_param_members.push_back(
-          ctx.dst->Member(name, type, std::move(attributes)));
+          ctx.dst->Member(symbol, type, std::move(attributes)));
-      return ctx.dst->MemberAccessor(InputStructSymbol(), name);
+      return ctx.dst->MemberAccessor(InputStructSymbol(), symbol);
     }
   }
 

src/transform/canonicalize_entry_point_io_test.cc

@@ -2062,18 +2062,31 @@ var<private> vertex_point_size : f32;
 var<private> vertex_point_size_1 : f32;
 var<private> vertex_point_size_2 : f32;
 
+struct VertIn1 {
+  [[location(0)]] collide : f32;
+};
+
+struct VertIn2 {
+  [[location(1)]] collide : f32;
+};
+
 struct VertOut {
   [[location(0)]] vertex_point_size : f32;
   [[builtin(position)]] vertex_point_size_1 : vec4<f32>;
 };
 
 [[stage(vertex)]]
-fn vert_main() -> VertOut {
+fn vert_main(collide : VertIn1, collide_1 : VertIn2) -> VertOut {
+  let x = collide.collide + collide_1.collide;
   return VertOut();
 }
 )";
 
   auto* expect = R"(
+[[location(0), internal(disable_validation__ignore_storage_class)]] var<in> collide_2 : f32;
+
+[[location(1), internal(disable_validation__ignore_storage_class)]] var<in> collide_3 : f32;
+
 [[location(0), internal(disable_validation__ignore_storage_class)]] var<out> vertex_point_size_3 : f32;
 
 [[builtin(position), internal(disable_validation__ignore_storage_class)]] var<out> vertex_point_size_1_1 : vec4<f32>;
@@ -2086,18 +2099,27 @@ var<private> vertex_point_size_1 : f32;
 
 var<private> vertex_point_size_2 : f32;
 
+struct VertIn1 {
+  collide : f32;
+};
+
+struct VertIn2 {
+  collide : f32;
+};
+
 struct VertOut {
   vertex_point_size : f32;
   vertex_point_size_1 : vec4<f32>;
 };
 
-fn vert_main_inner() -> VertOut {
+fn vert_main_inner(collide : VertIn1, collide_1 : VertIn2) -> VertOut {
+  let x = (collide.collide + collide_1.collide);
   return VertOut();
 }
 
 [[stage(vertex)]]
 fn vert_main() {
-  let inner_result = vert_main_inner();
+  let inner_result = vert_main_inner(VertIn1(collide_2), VertIn2(collide_3));
   vertex_point_size_3 = inner_result.vertex_point_size;
   vertex_point_size_1_1 = inner_result.vertex_point_size_1;
   vertex_point_size_4 = 1.0;
@@ -2114,24 +2136,48 @@ fn vert_main() {
 
 TEST_F(CanonicalizeEntryPointIOTest, EmitVertexPointSize_AvoidNameClash_Msl) {
   auto* src = R"(
+struct VertIn1 {
+  [[location(0)]] collide : f32;
+};
+
+struct VertIn2 {
+  [[location(1)]] collide : f32;
+};
+
 struct VertOut {
   [[location(0)]] vertex_point_size : vec4<f32>;
   [[builtin(position)]] vertex_point_size_1 : vec4<f32>;
 };
 
 [[stage(vertex)]]
-fn vert_main() -> VertOut {
+fn vert_main(collide : VertIn1, collide_1 : VertIn2) -> VertOut {
+  let x = collide.collide + collide_1.collide;
   return VertOut();
 }
 )";
 
   auto* expect = R"(
+struct VertIn1 {
+  collide : f32;
+};
+
+struct VertIn2 {
+  collide : f32;
+};
+
 struct VertOut {
   vertex_point_size : vec4<f32>;
   vertex_point_size_1 : vec4<f32>;
 };
 
-struct tint_symbol {
+struct tint_symbol_1 {
+  [[location(0)]]
+  collide : f32;
+  [[location(1)]]
+  collide_2 : f32;
+};
+
+struct tint_symbol_2 {
   [[location(0)]]
   vertex_point_size : vec4<f32>;
   [[builtin(position)]]
@@ -2140,14 +2186,15 @@ struct tint_symbol {
   vertex_point_size_2 : f32;
 };
 
-fn vert_main_inner() -> VertOut {
+fn vert_main_inner(collide : VertIn1, collide_1 : VertIn2) -> VertOut {
+  let x = (collide.collide + collide_1.collide);
   return VertOut();
 }
 
 [[stage(vertex)]]
-fn vert_main() -> tint_symbol {
+fn vert_main(tint_symbol : tint_symbol_1) -> tint_symbol_2 {
-  let inner_result = vert_main_inner();
+  let inner_result = vert_main_inner(VertIn1(tint_symbol.collide), VertIn2(tint_symbol.collide_2));
-  var wrapper_result : tint_symbol;
+  var wrapper_result : tint_symbol_2;
   wrapper_result.vertex_point_size = inner_result.vertex_point_size;
   wrapper_result.vertex_point_size_1 = inner_result.vertex_point_size_1;
   wrapper_result.vertex_point_size_2 = 1.0;
@@ -2163,6 +2210,82 @@ fn vert_main() -> tint_symbol {
   EXPECT_EQ(expect, str(got));
 }
 
+TEST_F(CanonicalizeEntryPointIOTest, EmitVertexPointSize_AvoidNameClash_Hlsl) {
+  auto* src = R"(
+struct VertIn1 {
+  [[location(0)]] collide : f32;
+};
+
+struct VertIn2 {
+  [[location(1)]] collide : f32;
+};
+
+struct VertOut {
+  [[location(0)]] vertex_point_size : vec4<f32>;
+  [[builtin(position)]] vertex_point_size_1 : vec4<f32>;
+};
+
+[[stage(vertex)]]
+fn vert_main(collide : VertIn1, collide_1 : VertIn2) -> VertOut {
+  let x = collide.collide + collide_1.collide;
+  return VertOut();
+}
+)";
+
+  auto* expect = R"(
+struct VertIn1 {
+  collide : f32;
+};
+
+struct VertIn2 {
+  collide : f32;
+};
+
+struct VertOut {
+  vertex_point_size : vec4<f32>;
+  vertex_point_size_1 : vec4<f32>;
+};
+
+struct tint_symbol_1 {
+  [[location(0)]]
+  collide : f32;
+  [[location(1)]]
+  collide_2 : f32;
+};
+
+struct tint_symbol_2 {
+  [[location(0)]]
+  vertex_point_size : vec4<f32>;
+  [[builtin(position)]]
+  vertex_point_size_1 : vec4<f32>;
+  [[builtin(pointsize)]]
+  vertex_point_size_2 : f32;
+};
+
+fn vert_main_inner(collide : VertIn1, collide_1 : VertIn2) -> VertOut {
+  let x = (collide.collide + collide_1.collide);
+  return VertOut();
+}
+
+[[stage(vertex)]]
+fn vert_main(tint_symbol : tint_symbol_1) -> tint_symbol_2 {
+  let inner_result = vert_main_inner(VertIn1(tint_symbol.collide), VertIn2(tint_symbol.collide_2));
+  var wrapper_result : tint_symbol_2;
+  wrapper_result.vertex_point_size = inner_result.vertex_point_size;
+  wrapper_result.vertex_point_size_1 = inner_result.vertex_point_size_1;
+  wrapper_result.vertex_point_size_2 = 1.0;
+  return wrapper_result;
+}
+)";
+
+  DataMap data;
+  data.Add<CanonicalizeEntryPointIO::Config>(
+      CanonicalizeEntryPointIO::ShaderStyle::kHlsl, 0xFFFFFFFF, true);
+  auto got = Run<CanonicalizeEntryPointIO>(src, data);
+
+  EXPECT_EQ(expect, str(got));
+}
+
 TEST_F(CanonicalizeEntryPointIOTest, SpirvSampleMaskBuiltins) {
   auto* src = R"(
 [[stage(fragment)]]

test/bug/chromium/1251009.wgsl

@@ -0,0 +1,19 @@
+struct VertexInputs0 {
+  [[builtin(vertex_index)]] vertex_index : u32;
+  [[location(0)]] loc0 : i32;
+};
+struct VertexInputs1 {
+  [[location(2)]] loc1 : u32;
+  [[location(3)]] loc3 : vec4<f32>;
+};
+
+[[stage(vertex)]]
+fn main(
+  inputs0 : VertexInputs0,
+  [[location(1)]] loc1 : u32,
+  [[builtin(instance_index)]] instance_index : u32,
+  inputs1 : VertexInputs1,
+) -> [[builtin(position)]] vec4<f32> {
+  let foo : u32 = inputs0.vertex_index + instance_index;
+  return vec4<f32>();
+}

test/bug/chromium/1251009.wgsl.expected.hlsl

@@ -0,0 +1,33 @@
+struct VertexInputs0 {
+  uint vertex_index;
+  int loc0;
+};
+struct VertexInputs1 {
+  uint loc1;
+  float4 loc3;
+};
+struct tint_symbol_1 {
+  int loc0 : TEXCOORD0;
+  uint loc1 : TEXCOORD1;
+  uint loc1_1 : TEXCOORD2;
+  float4 loc3 : TEXCOORD3;
+  uint vertex_index : SV_VertexID;
+  uint instance_index : SV_InstanceID;
+};
+struct tint_symbol_2 {
+  float4 value : SV_Position;
+};
+
+float4 main_inner(VertexInputs0 inputs0, uint loc1, uint instance_index, VertexInputs1 inputs1) {
+  const uint foo = (inputs0.vertex_index + instance_index);
+  return float4(0.0f, 0.0f, 0.0f, 0.0f);
+}
+
+tint_symbol_2 main(tint_symbol_1 tint_symbol) {
+  const VertexInputs0 tint_symbol_3 = {tint_symbol.vertex_index, tint_symbol.loc0};
+  const VertexInputs1 tint_symbol_4 = {tint_symbol.loc1_1, tint_symbol.loc3};
+  const float4 inner_result = main_inner(tint_symbol_3, tint_symbol.loc1, tint_symbol.instance_index, tint_symbol_4);
+  tint_symbol_2 wrapper_result = (tint_symbol_2)0;
+  wrapper_result.value = inner_result;
+  return wrapper_result;
+}

test/bug/chromium/1251009.wgsl.expected.msl

@@ -0,0 +1,35 @@
+#include <metal_stdlib>
+
+using namespace metal;
+struct VertexInputs0 {
+  uint vertex_index;
+  int loc0;
+};
+struct VertexInputs1 {
+  uint loc1;
+  float4 loc3;
+};
+struct tint_symbol_2 {
+  int loc0 [[attribute(0)]];
+  uint loc1 [[attribute(1)]];
+  uint loc1_1 [[attribute(2)]];
+  float4 loc3 [[attribute(3)]];
+};
+struct tint_symbol_3 {
+  float4 value [[position]];
+};
+
+float4 tint_symbol_inner(VertexInputs0 inputs0, uint loc1, uint instance_index, VertexInputs1 inputs1) {
+  uint const foo = (inputs0.vertex_index + instance_index);
+  return float4();
+}
+
+vertex tint_symbol_3 tint_symbol(uint vertex_index [[vertex_id]], uint instance_index [[instance_id]], tint_symbol_2 tint_symbol_1 [[stage_in]]) {
+  VertexInputs0 const tint_symbol_4 = {.vertex_index=vertex_index, .loc0=tint_symbol_1.loc0};
+  VertexInputs1 const tint_symbol_5 = {.loc1=tint_symbol_1.loc1_1, .loc3=tint_symbol_1.loc3};
+  float4 const inner_result = tint_symbol_inner(tint_symbol_4, tint_symbol_1.loc1, instance_index, tint_symbol_5);
+  tint_symbol_3 wrapper_result = {};
+  wrapper_result.value = inner_result;
+  return wrapper_result;
+}
+

test/bug/chromium/1251009.wgsl.expected.spvasm

@@ -0,0 +1,90 @@
+; SPIR-V
+; Version: 1.3
+; Generator: Google Tint Compiler; 0
+; Bound: 45
+; Schema: 0
+               OpCapability Shader
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Vertex %main "main" %vertex_index_1 %loc0_1 %loc1_1 %instance_index_1 %loc1_2 %loc3_1 %value %vertex_point_size
+               OpName %vertex_index_1 "vertex_index_1"
+               OpName %loc0_1 "loc0_1"
+               OpName %loc1_1 "loc1_1"
+               OpName %instance_index_1 "instance_index_1"
+               OpName %loc1_2 "loc1_2"
+               OpName %loc3_1 "loc3_1"
+               OpName %value "value"
+               OpName %vertex_point_size "vertex_point_size"
+               OpName %VertexInputs0 "VertexInputs0"
+               OpMemberName %VertexInputs0 0 "vertex_index"
+               OpMemberName %VertexInputs0 1 "loc0"
+               OpName %VertexInputs1 "VertexInputs1"
+               OpMemberName %VertexInputs1 0 "loc1"
+               OpMemberName %VertexInputs1 1 "loc3"
+               OpName %main_inner "main_inner"
+               OpName %inputs0 "inputs0"
+               OpName %loc1 "loc1"
+               OpName %instance_index "instance_index"
+               OpName %inputs1 "inputs1"
+               OpName %main "main"
+               OpDecorate %vertex_index_1 BuiltIn VertexIndex
+               OpDecorate %loc0_1 Location 0
+               OpDecorate %loc1_1 Location 1
+               OpDecorate %instance_index_1 BuiltIn InstanceIndex
+               OpDecorate %loc1_2 Location 2
+               OpDecorate %loc3_1 Location 3
+               OpDecorate %value BuiltIn Position
+               OpDecorate %vertex_point_size BuiltIn PointSize
+               OpMemberDecorate %VertexInputs0 0 Offset 0
+               OpMemberDecorate %VertexInputs0 1 Offset 4
+               OpMemberDecorate %VertexInputs1 0 Offset 0
+               OpMemberDecorate %VertexInputs1 1 Offset 16
+       %uint = OpTypeInt 32 0
+%_ptr_Input_uint = OpTypePointer Input %uint
+%vertex_index_1 = OpVariable %_ptr_Input_uint Input
+        %int = OpTypeInt 32 1
+%_ptr_Input_int = OpTypePointer Input %int
+     %loc0_1 = OpVariable %_ptr_Input_int Input
+     %loc1_1 = OpVariable %_ptr_Input_uint Input
+%instance_index_1 = OpVariable %_ptr_Input_uint Input
+     %loc1_2 = OpVariable %_ptr_Input_uint Input
+      %float = OpTypeFloat 32
+    %v4float = OpTypeVector %float 4
+%_ptr_Input_v4float = OpTypePointer Input %v4float
+     %loc3_1 = OpVariable %_ptr_Input_v4float Input
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+         %16 = OpConstantNull %v4float
+      %value = OpVariable %_ptr_Output_v4float Output %16
+%_ptr_Output_float = OpTypePointer Output %float
+         %19 = OpConstantNull %float
+%vertex_point_size = OpVariable %_ptr_Output_float Output %19
+%VertexInputs0 = OpTypeStruct %uint %int
+%VertexInputs1 = OpTypeStruct %uint %v4float
+         %20 = OpTypeFunction %v4float %VertexInputs0 %uint %uint %VertexInputs1
+       %void = OpTypeVoid
+         %31 = OpTypeFunction %void
+    %float_1 = OpConstant %float 1
+ %main_inner = OpFunction %v4float None %20
+    %inputs0 = OpFunctionParameter %VertexInputs0
+       %loc1 = OpFunctionParameter %uint
+%instance_index = OpFunctionParameter %uint
+    %inputs1 = OpFunctionParameter %VertexInputs1
+         %28 = OpLabel
+         %29 = OpCompositeExtract %uint %inputs0 0
+         %30 = OpIAdd %uint %29 %instance_index
+               OpReturnValue %16
+               OpFunctionEnd
+       %main = OpFunction %void None %31
+         %34 = OpLabel
+         %36 = OpLoad %uint %vertex_index_1
+         %37 = OpLoad %int %loc0_1
+         %38 = OpCompositeConstruct %VertexInputs0 %36 %37
+         %39 = OpLoad %uint %loc1_1
+         %40 = OpLoad %uint %instance_index_1
+         %41 = OpLoad %uint %loc1_2
+         %42 = OpLoad %v4float %loc3_1
+         %43 = OpCompositeConstruct %VertexInputs1 %41 %42
+         %35 = OpFunctionCall %v4float %main_inner %38 %39 %40 %43
+               OpStore %value %35
+               OpStore %vertex_point_size %float_1
+               OpReturn
+               OpFunctionEnd

test/bug/chromium/1251009.wgsl.expected.wgsl

@@ -0,0 +1,19 @@
+struct VertexInputs0 {
+  [[builtin(vertex_index)]]
+  vertex_index : u32;
+  [[location(0)]]
+  loc0 : i32;
+};
+
+struct VertexInputs1 {
+  [[location(2)]]
+  loc1 : u32;
+  [[location(3)]]
+  loc3 : vec4<f32>;
+};
+
+[[stage(vertex)]]
+fn main(inputs0 : VertexInputs0, [[location(1)]] loc1 : u32, [[builtin(instance_index)]] instance_index : u32, inputs1 : VertexInputs1) -> [[builtin(position)]] vec4<f32> {
+  let foo : u32 = (inputs0.vertex_index + instance_index);
+  return vec4<f32>();
+}