[spirv] Avoid unifying struct types with different names (#1367)

We were unifying struct types as long as they have the exact
same members (regardless of names). BUt it turns out some
reflection workflow requires the exact original struct type
to work.

tools/clang/include/clang/SPIRV/Type.h

@@ -18,6 +18,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace spirv {
@@ -95,7 +96,7 @@ public:
                                      DecorationSet decs = {});
   static const Type *getStruct(SPIRVContext &ctx,
                                llvm::ArrayRef<uint32_t> members,
-                               DecorationSet d = {});
+                               llvm::StringRef name = {}, DecorationSet d = {});
   static const Type *getPointer(SPIRVContext &ctx,
                                 spv::StorageClass storage_class, uint32_t type,
                                 DecorationSet decs = {});
@@ -109,7 +110,8 @@ public:
 
 private:
   /// \brief Private constructor.
-  Type(spv::Op op, std::vector<uint32_t> arg = {}, DecorationSet dec = {});
+  Type(spv::Op op, std::vector<uint32_t> arg = {}, DecorationSet dec = {},
+       llvm::StringRef name = {});
 
   /// \brief Returns the unique Type pointer within the given context.
   static const Type *getUniqueType(SPIRVContext &, const Type &);
@@ -117,6 +119,7 @@ private:
 private:
   spv::Op opcode;             ///< OpCode of the Type defined in SPIR-V Spec
   std::vector<uint32_t> args; ///< Arguments needed to define the type
+  std::string name;           ///< Source code name of this type
 
   /// The decorations that are applied to a type.
   /// Note: we use a SetVector because:

tools/clang/lib/SPIRV/ModuleBuilder.cpp

@@ -1026,7 +1026,8 @@ ModuleBuilder::getStructType(llvm::ArrayRef<uint32_t> fieldTypes,
                              llvm::StringRef structName,
                              llvm::ArrayRef<llvm::StringRef> fieldNames,
                              Type::DecorationSet decorations) {
-  const Type *type = Type::getStruct(theContext, fieldTypes, decorations);
+  const Type *type =
+      Type::getStruct(theContext, fieldTypes, structName, decorations);
   bool isRegistered = false;
   const uint32_t typeId = theContext.getResultIdForType(type, &isRegistered);
   theModule.addType(type, typeId);
@@ -1112,7 +1113,8 @@ uint32_t ModuleBuilder::getImageType(uint32_t sampledType, spv::Dim dim,
     requireCapability(spv::Capability::StorageImageExtendedFormats);
     break;
   default:
-    // Only image formats requiring extended formats are relevant. The rest just pass through.
+    // Only image formats requiring extended formats are relevant. The rest just
+    // pass through.
     break;
   }
 
@@ -1204,7 +1206,7 @@ uint32_t ModuleBuilder::getByteAddressBufferType(bool isRW) {
   if (!isRW)
     typeDecs.push_back(Decoration::getNonWritable(theContext, 0));
 
-  const Type *type = Type::getStruct(theContext, {raTypeId}, typeDecs);
+  const Type *type = Type::getStruct(theContext, {raTypeId}, "", typeDecs);
   const uint32_t typeId = theContext.getResultIdForType(type);
   theModule.addType(type, typeId);
   theModule.addDebugName(typeId, isRW ? "type.RWByteAddressBuffer"

tools/clang/lib/SPIRV/Type.cpp

@@ -14,8 +14,9 @@
 namespace clang {
 namespace spirv {
 
-Type::Type(spv::Op op, std::vector<uint32_t> arg, DecorationSet decs)
-    : opcode(op), args(std::move(arg)) {
+Type::Type(spv::Op op, std::vector<uint32_t> arg, DecorationSet decs,
+           llvm::StringRef n)
+    : opcode(op), args(std::move(arg)), name(n.str()) {
   decorations = llvm::SetVector<const Decoration *>(decs.begin(), decs.end());
 }
 
@@ -126,8 +127,9 @@ const Type *Type::getRuntimeArray(SPIRVContext &context,
   return getUniqueType(context, t);
 }
 const Type *Type::getStruct(SPIRVContext &context,
-                            llvm::ArrayRef<uint32_t> members, DecorationSet d) {
-  Type t = Type(spv::Op::OpTypeStruct, std::vector<uint32_t>(members), d);
+                            llvm::ArrayRef<uint32_t> members,
+                            llvm::StringRef name, DecorationSet d) {
+  Type t = Type(spv::Op::OpTypeStruct, std::vector<uint32_t>(members), d, name);
   return getUniqueType(context, t);
 }
 const Type *Type::getPointer(SPIRVContext &context,
@@ -148,7 +150,7 @@ const Type *Type::getFunction(SPIRVContext &context, uint32_t return_type,
 
 bool Type::operator==(const Type &other) const {
   if (opcode == other.opcode && args == other.args &&
-      decorations.size() == other.decorations.size()) {
+      decorations.size() == other.decorations.size() && name == other.name) {
     // If two types have the same decorations, but in different order,
     // they are in fact the same type.
     for (const Decoration *dec : decorations) {

tools/clang/test/CodeGenSPIRV/empty-struct-interface.vs.hlsl2spv

@@ -22,25 +22,28 @@ VSOut main(VSIn input)
 // OpName %main "main"
 // OpName %VSIn "VSIn"
 // OpName %param_var_input "param.var.input"
+// OpName %VSOut "VSOut"
 // OpName %input "input"
 // OpName %result "result"
 // %void = OpTypeVoid
 // %3 = OpTypeFunction %void
 // %VSIn = OpTypeStruct
 // %_ptr_Function_VSIn = OpTypePointer Function %VSIn
-// %11 = OpTypeFunction %VSIn %_ptr_Function_VSIn
+// %VSOut = OpTypeStruct
+// %12 = OpTypeFunction %VSOut %_ptr_Function_VSIn
+// %_ptr_Function_VSOut = OpTypePointer Function %VSOut
 // %main = OpFunction %void None %3
 // %5 = OpLabel
 // %param_var_input = OpVariable %_ptr_Function_VSIn Function
 // %9 = OpCompositeConstruct %VSIn
 // OpStore %param_var_input %9
-// %10 = OpFunctionCall %VSIn %src_main %param_var_input
+// %11 = OpFunctionCall %VSOut %src_main %param_var_input
 // OpReturn
 // OpFunctionEnd
-// %src_main = OpFunction %VSIn None %11
+// %src_main = OpFunction %VSOut None %12
 // %input = OpFunctionParameter %_ptr_Function_VSIn
 // %bb_entry = OpLabel
-// %result = OpVariable %_ptr_Function_VSIn Function
-// %15 = OpLoad %VSIn %result
-// OpReturnValue %15
+// %result = OpVariable %_ptr_Function_VSOut Function
+// %17 = OpLoad %VSOut %result
+// OpReturnValue %17
 // OpFunctionEnd

tools/clang/test/CodeGenSPIRV/spirv.legal.sbuffer.counter.method.hlsl

@@ -65,7 +65,7 @@ float main() : VVV {
 // CHECK-NEXT:                    OpStore %counter_var_getCSBuffer_this_1_1 [[counter]]
 // CHECK-NEXT: [[counter:%\d+]] = OpLoad %_ptr_Uniform_type_ACSBuffer_counter %counter_var_localBundle_1_2
 // CHECK-NEXT:                    OpStore %counter_var_getCSBuffer_this_1_2 [[counter]]
-// CHECK-NEXT:                    OpFunctionCall %_ptr_Uniform_type_RWStructuredBuffer_float %TwoBundle_getCSBuffer %localBundle
+// CHECK-NEXT:                    OpFunctionCall %_ptr_Uniform_type_ConsumeStructuredBuffer_float %TwoBundle_getCSBuffer %localBundle
     float value = localBundle.getCSBuffer().Consume();
 
 // CHECK:      [[counter:%\d+]] = OpLoad %_ptr_Uniform_type_ACSBuffer_counter %counter_var_localWrapper_0_0_0
@@ -80,7 +80,7 @@ float main() : VVV {
 // CHECK-NEXT:                    OpStore %counter_var_getASBuffer_this_0_1_1 [[counter]]
 // CHECK-NEXT: [[counter:%\d+]] = OpLoad %_ptr_Uniform_type_ACSBuffer_counter %counter_var_localWrapper_0_1_2
 // CHECK-NEXT:                    OpStore %counter_var_getASBuffer_this_0_1_2 [[counter]]
-// CHECK-NEXT:                    OpFunctionCall %_ptr_Uniform_type_RWStructuredBuffer_float %Wrapper_getASBuffer %localWrapper
+// CHECK-NEXT:                    OpFunctionCall %_ptr_Uniform_type_AppendStructuredBuffer_float %Wrapper_getASBuffer %localWrapper
     localWrapper.getASBuffer().Append(4.2);
 
 // CHECK:      [[counter:%\d+]] = OpLoad %_ptr_Uniform_type_ACSBuffer_counter %counter_var_localWrapper_0_0_0

tools/clang/test/CodeGenSPIRV/spirv.legal.sbuffer.struct.hlsl

@@ -5,7 +5,7 @@ struct Basic {
     float4 b;
 };
 
-// CHECK: %S = OpTypeStruct %_ptr_Uniform_type_AppendStructuredBuffer_v4float %_ptr_Uniform_type_AppendStructuredBuffer_v4float
+// CHECK: %S = OpTypeStruct %_ptr_Uniform_type_AppendStructuredBuffer_v4float %_ptr_Uniform_type_ConsumeStructuredBuffer_v4float
 struct S {
      AppendStructuredBuffer<float4> append;
     ConsumeStructuredBuffer<float4> consume;
@@ -52,7 +52,7 @@ float4 main() : SV_Target {
 // CHECK:      [[ptr:%\d+]] = OpAccessChain %_ptr_Function__ptr_Uniform_type_AppendStructuredBuffer_v4float %c %int_0 %int_0
 // CHECK-NEXT:                OpStore [[ptr]] %gASBuffer
     c.s.append = gASBuffer;
-// CHECK:      [[ptr:%\d+]] = OpAccessChain %_ptr_Function__ptr_Uniform_type_AppendStructuredBuffer_v4float %c %int_0 %int_1
+// CHECK:      [[ptr:%\d+]] = OpAccessChain %_ptr_Function__ptr_Uniform_type_ConsumeStructuredBuffer_v4float %c %int_0 %int_1
 // CHECK-NEXT:                OpStore [[ptr]] %gCSBuffer
     c.s.consume = gCSBuffer;
 

tools/clang/test/CodeGenSPIRV/type.struct.uniqueness.hlsl

@@ -0,0 +1,45 @@
+// Run: %dxc -T vs_6_0 -E main
+
+// CHECK: %type_MyCBuffer = OpTypeStruct %v4float
+cbuffer MyCBuffer {
+    float4 CB_val;
+}
+
+// CHECK: %type_ConstantBuffer_S = OpTypeStruct %v4float
+// CHECK: %S = OpTypeStruct %v4float
+struct S {
+    float4 val;
+};
+ConstantBuffer<S>          MyConstantBuffer;
+
+// CHECK: %type_AppendStructuredBuffer_S = OpTypeStruct %_runtimearr_S
+AppendStructuredBuffer<S>  MyASBuffer;
+// CHECK: %type_ConsumeStructuredBuffer_S = OpTypeStruct %_runtimearr_S
+ConsumeStructuredBuffer<S> MyCSBuffer;
+
+// CHECK: %type__Globals = OpTypeStruct %v4float
+
+// CHECK: %Empty1 = OpTypeStruct
+// CHECK: %Empty2 = OpTypeStruct
+struct Empty1 {};
+struct Empty2 {};
+
+// CHECK: %OneField1 = OpTypeStruct %v4float
+// CHECK: %OneField2 = OpTypeStruct %v4float
+struct OneField1 {
+    float4 val;
+};
+struct OneField2 {
+    float4 val;
+};
+
+float4 val; // Compose $Globals
+
+
+
+void main() {
+    Empty1 e1;
+    Empty2 e2;
+    OneField1 of1;
+    OneField2 of2;
+}

tools/clang/test/CodeGenSPIRV/vk.cloption.invert-y.ds.hlsl

@@ -24,7 +24,7 @@ DsCpOut main(OutputPatch<DsCpIn, 3> patch,
   return dsOut;
 }
 
-// CHECK:      [[call:%\d+]] = OpFunctionCall %DsCpIn %src_main %param_var_patch %param_var_pcfData
+// CHECK:      [[call:%\d+]] = OpFunctionCall %DsCpOut %src_main %param_var_patch %param_var_pcfData
 // CHECK-NEXT:  [[val:%\d+]] = OpCompositeExtract %v4float [[call]] 0
 // CHECK-NEXT: [[oldY:%\d+]] = OpCompositeExtract %float [[val]] 1
 // CHECK-NEXT: [[newY:%\d+]] = OpFNegate %float [[oldY]]

tools/clang/test/CodeGenSPIRV/vk.cloption.invert-y.gs.hlsl

@@ -16,7 +16,7 @@ void main(in    line GsVIn              inData[2],
 
     GsVOut vertex;
     vertex = (GsVOut)0;
-// CHECK:      [[vert:%\d+]] = OpLoad %GsVIn %vertex
+// CHECK:      [[vert:%\d+]] = OpLoad %GsVOut %vertex
 // CHECK-NEXT:  [[val:%\d+]] = OpCompositeExtract %v4float [[vert]] 0
 // CHECK-NEXT: [[oldY:%\d+]] = OpCompositeExtract %float [[val]] 1
 // CHECK-NEXT: [[newY:%\d+]] = OpFNegate %float [[oldY]]

tools/clang/test/CodeGenSPIRV/vk.layout.cbuffer.nested.empty.std140.hlsl

@@ -5,7 +5,8 @@
 // CHECK: OpMemberDecorate %type_Params_cbuffer 0 Offset 0
 
 // CHECK:        %InnerStruct0 = OpTypeStruct
-// CHECK:         %OuterStruct = OpTypeStruct %InnerStruct0 %InnerStruct0
+// CHECK:        %InnerStruct1 = OpTypeStruct
+// CHECK:         %OuterStruct = OpTypeStruct %InnerStruct0 %InnerStruct1
 // CHECK: %type_Params_cbuffer = OpTypeStruct %OuterStruct
 
 struct InnerStruct0 {};

tools/clang/test/CodeGenSPIRV/vk.layout.vector.relaxed.hlsl

@@ -58,12 +58,13 @@
 // CHECK:                         %T = OpTypeStruct %S_0
 // CHECK:   %type_StructuredBuffer_T = OpTypeStruct %_runtimearr_T
 // CHECK: %type_RWStructuredBuffer_T = OpTypeStruct %_runtimearr_T
+// CHECK:              %type_CBuffer = OpTypeStruct %S
 // CHECK:              %type_TBuffer = OpTypeStruct %S_0
 
 // CHECK:   %MyCBuffer = OpVariable %_ptr_Uniform_type_ConstantBuffer_T Uniform
 // CHECK:   %MySBuffer = OpVariable %_ptr_Uniform_type_StructuredBuffer_T Uniform
 // CHECK: %MyRWSBuffer = OpVariable %_ptr_Uniform_type_RWStructuredBuffer_T Uniform
-// CHECK:     %CBuffer = OpVariable %_ptr_Uniform_type_ConstantBuffer_T Uniform
+// CHECK:     %CBuffer = OpVariable %_ptr_Uniform_type_CBuffer Uniform
 // CHECK:     %TBuffer = OpVariable %_ptr_Uniform_type_TBuffer Uniform
 
 struct S {

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -52,6 +52,9 @@ TEST_F(FileTest, MatrixTypesMajornessZpc) {
   runFileTest("type.matrix.majorness.zpc.hlsl");
 }
 TEST_F(FileTest, StructTypes) { runFileTest("type.struct.hlsl"); }
+TEST_F(FileTest, StructTypeUniqueness) {
+  runFileTest("type.struct.uniqueness.hlsl");
+}
 TEST_F(FileTest, ClassTypes) { runFileTest("type.class.hlsl"); }
 TEST_F(FileTest, ArrayTypes) { runFileTest("type.array.hlsl"); }
 TEST_F(FileTest, RuntimeArrayTypes) { runFileTest("type.runtime-array.hlsl"); }

tools/clang/unittests/SPIRV/SPIRVContextTest.cpp

@@ -7,10 +7,10 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "gmock/gmock.h"
-#include "clang/SPIRV/Decoration.h"
 #include "clang/SPIRV/SPIRVContext.h"
+#include "clang/SPIRV/Decoration.h"
 #include "clang/SPIRV/Type.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace clang::spirv;
@@ -61,18 +61,26 @@ TEST(SPIRVContext, UniqueIdForUniqueAggregateType) {
       Decoration::getBuiltIn(ctx, spv::BuiltIn::Position, 0);
 
   const Type *struct_1 = Type::getStruct(
-      ctx, {intt_id, boolt_id},
+      ctx, {intt_id, boolt_id}, "struct1",
       {relaxed, bufferblock, mem_0_offset, mem_1_offset, mem_0_position});
 
   const Type *struct_2 = Type::getStruct(
-      ctx, {intt_id, boolt_id},
+      ctx, {intt_id, boolt_id}, "struct1",
+      {relaxed, bufferblock, mem_0_offset, mem_1_offset, mem_0_position});
+
+  const Type *struct_3 = Type::getStruct(
+      ctx, {intt_id, boolt_id}, "struct2",
       {relaxed, bufferblock, mem_0_offset, mem_1_offset, mem_0_position});
 
   const uint32_t struct_1_id = ctx.getResultIdForType(struct_1);
   const uint32_t struct_2_id = ctx.getResultIdForType(struct_2);
+  const uint32_t struct_3_id = ctx.getResultIdForType(struct_3);
 
   // We should be able to retrieve the same ID for the same Type.
   EXPECT_EQ(struct_1_id, struct_2_id);
+
+  // Name matters.
+  EXPECT_NE(struct_1_id, struct_3_id);
 }
 
 TEST(SPIRVContext, UniqueIdForUniqueConstants) {

tools/clang/unittests/SPIRV/TypeTest.cpp

@@ -7,11 +7,11 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/SPIRV/Type.h"
 #include "SPIRVTestUtils.h"
-#include "gmock/gmock.h"
 #include "clang/SPIRV/SPIRVContext.h"
 #include "clang/SPIRV/String.h"
-#include "clang/SPIRV/Type.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace clang::spirv;
@@ -47,15 +47,19 @@ TEST(Type, SameAggregateTypeWithDecorationsShouldHaveSameAddress) {
       Decoration::getBuiltIn(ctx, spv::BuiltIn::Position, 0);
 
   const Type *struct_1 = Type::getStruct(
-      ctx, {intt_id, boolt_id},
+      ctx, {intt_id, boolt_id}, "",
       {relaxed, bufferblock, mem_0_offset, mem_1_offset, mem_0_position});
 
   const Type *struct_2 = Type::getStruct(
-      ctx, {intt_id, boolt_id},
+      ctx, {intt_id, boolt_id}, "",
       {relaxed, bufferblock, mem_0_offset, mem_1_offset, mem_0_position});
 
   const Type *struct_3 = Type::getStruct(
-      ctx, {intt_id, boolt_id},
+      ctx, {intt_id, boolt_id}, "",
+      {bufferblock, mem_0_offset, mem_0_position, mem_1_offset, relaxed});
+
+  const Type *struct_4 = Type::getStruct(
+      ctx, {intt_id, boolt_id}, "name",
       {bufferblock, mem_0_offset, mem_0_position, mem_1_offset, relaxed});
 
   // 2 types with the same signature. We should get the same pointer.
@@ -63,6 +67,9 @@ TEST(Type, SameAggregateTypeWithDecorationsShouldHaveSameAddress) {
 
   // The order of decorations does not matter.
   EXPECT_EQ(struct_1, struct_3);
+
+  // Struct with different names are different.
+  EXPECT_NE(struct_3, struct_4);
 }
 
 TEST(Type, Void) {
@@ -354,7 +361,7 @@ TEST(Type, StructBasic) {
 TEST(Type, StructWithDecoration) {
   SPIRVContext ctx;
   const Decoration *bufferblock = Decoration::getBufferBlock(ctx);
-  const Type *t = Type::getStruct(ctx, {2, 3, 4}, {bufferblock});
+  const Type *t = Type::getStruct(ctx, {2, 3, 4}, "", {bufferblock});
   EXPECT_EQ(t->getOpcode(), spv::Op::OpTypeStruct);
   EXPECT_THAT(t->getArgs(), ElementsAre(2, 3, 4));
   EXPECT_THAT(t->getDecorations(), ElementsAre(bufferblock));
@@ -370,7 +377,7 @@ TEST(Type, StructWithDecoratedMembers) {
       Decoration::getBuiltIn(ctx, spv::BuiltIn::Position, 0);
 
   const Type *t = Type::getStruct(
-      ctx, {2, 3, 4},
+      ctx, {2, 3, 4}, "",
       {relaxed, bufferblock, mem_0_position, mem_0_offset, mem_1_offset});
   EXPECT_EQ(t->getOpcode(), spv::Op::OpTypeStruct);
   EXPECT_THAT(t->getArgs(), ElementsAre(2, 3, 4));