Fix debug info offsets for vectors with 16-bit types (#6775)

This fixes a bug where the offsets for elements in vectors with 16-bit
types doesn't take into account alignment bits and PIX wouldn't display
vector element values correctly in the shader debugger. Eg. if
`-enable-16bit-types` wasn't set, the offsets for a min16float4 would be
0, 16, 32, 48 instead of 0, 32, 64, 96.

Also removed the assert in PopulateAllocaMap_StructType that was
checking whether the calculated aligned offset matches the packed offset
(from SortedMembers) because it was false for members with sizes smaller
than the alignment size.

(cherry picked from commit 84c0a09)

lib/DxilPIXPasses/DxilDbgValueToDbgDeclare.cpp

@@ -138,28 +138,8 @@ class OffsetManager {
     unsigned AlignMask = DescendTypeToGetAlignMask(Ty);
     if (AlignMask) {
       VALUE_TO_DECLARE_LOG("Aligning to %d", AlignMask);
-      // This is some magic arithmetic. Here's an example:
-      //
-      // Assume the natural alignment for Ty is 16 bits. Then
-      //
-      //     AlignMask = 0x0000000f(15)
-      //
-      // If the current aligned offset is
-      //
-      //     CurrentAlignedOffset = 0x00000048(72)
-      //
-      // Then
-      //
-      //     T = CurrentAlignOffset + AlignMask = 0x00000057(87)
-      //
-      // Which mean
-      //
-      //     T & ~CurrentOffset = 0x00000050(80)
-      //
-      // is the aligned offset where Ty should be placed.
-      AlignMask = AlignMask - 1;
       m_CurrentAlignedOffset =
-          (m_CurrentAlignedOffset + AlignMask) & ~AlignMask;
+          llvm::RoundUpToAlignment(m_CurrentAlignedOffset, AlignMask);
     } else {
       VALUE_TO_DECLARE_LOG("Failed to find alignment");
     }
@@ -1317,7 +1297,7 @@ void VariableRegisters::PopulateAllocaMap_StructType(
   const llvm::DITypeIdentifierMap EmptyMap;
 
   for (auto OffsetAndMember : SortedMembers) {
-    VALUE_TO_DECLARE_LOG("Member: %s at aligned offset %d",
+    VALUE_TO_DECLARE_LOG("Member: %s at packed offset %d",
                          OffsetAndMember.second->getName().str().c_str(),
                          OffsetAndMember.first);
     // Align the offsets to the member's type natural alignment. This
@@ -1331,9 +1311,12 @@ void VariableRegisters::PopulateAllocaMap_StructType(
       // size would be lost
       PopulateAllocaMap(OffsetAndMember.second);
     } else {
-      assert(m_Offsets.GetCurrentAlignedOffset() ==
-                 StructStart + OffsetAndMember.first &&
-             "Offset mismatch in DIStructType");
+      if (OffsetAndMember.second->getAlignInBits() ==
+          OffsetAndMember.second->getSizeInBits()) {
+        assert(m_Offsets.GetCurrentAlignedOffset() ==
+                   StructStart + OffsetAndMember.first &&
+               "Offset mismatch in DIStructType");
+      }
       if (IsResourceObject(OffsetAndMember.second)) {
         m_Offsets.AddResourceType(OffsetAndMember.second);
       } else {

tools/clang/lib/CodeGen/CGDebugInfo.cpp

@@ -1043,14 +1043,19 @@ bool CGDebugInfo::TryCollectHLSLRecordElements(const RecordType *Ty,
     // extended vector type, which is represented as an array in DWARF.
     // However, we logically represent it as one field per component.
     QualType ElemQualTy = hlsl::GetHLSLVecElementType(QualTy);
+    unsigned AlignBits = CGM.getContext().getTypeAlign(ElemQualTy);
     unsigned VecSize = hlsl::GetHLSLVecSize(QualTy);
     unsigned ElemSizeInBits = CGM.getContext().getTypeSize(ElemQualTy);
+    unsigned CurrentAlignedOffset = 0;
     for (unsigned ElemIdx = 0; ElemIdx < VecSize; ++ElemIdx) {
       StringRef FieldName = StringRef(&"xyzw"[ElemIdx], 1);
-      unsigned OffsetInBits = ElemSizeInBits * ElemIdx;
-      llvm::DIType *FieldType = createFieldType(FieldName, ElemQualTy, 0,
-        SourceLocation(), AccessSpecifier::AS_public, OffsetInBits,
-        /* tunit */ nullptr, DITy, Ty->getDecl());
+      CurrentAlignedOffset =
+          llvm::RoundUpToAlignment(CurrentAlignedOffset, AlignBits);
+      llvm::DIType *FieldType =
+          createFieldType(FieldName, ElemQualTy, 0, SourceLocation(),
+                          AccessSpecifier::AS_public, CurrentAlignedOffset,
+                          /* tunit */ nullptr, DITy, Ty->getDecl());
+      CurrentAlignedOffset += ElemSizeInBits;
       Elements.emplace_back(FieldType);
     }
 

tools/clang/test/DXC/min16vector_disabled.hlsl

@@ -0,0 +1,19 @@
+// Validate that the offsets for a min16float vector is correct if 16-bit types aren't enabled
+// RUN: %dxc -T cs_6_5 -Od -Zi -Qembed_debug %s | FileCheck %s
+
+// CHECK-NOT: {{.*}}!DIDerivedType{{.*}}name: "x"{{.*}}offset: {{.*}}
+// CHECK: {{.*}}!DIDerivedType{{.*}}name: "y"{{.*}}offset: 32{{.*}}
+// CHECK: {{.*}}!DIDerivedType{{.*}}name: "z"{{.*}}offset: 64{{.*}}
+// CHECK: {{.*}}!DIDerivedType{{.*}}name: "w"{{.*}}offset: 96{{.*}}
+
+RWStructuredBuffer<int> UAV : register(u0);
+
+[numthreads(1, 1, 1)] 
+void main() {
+    min16float4 vector;
+    vector.x = UAV[0];
+    vector.y = UAV[1];
+    vector.z = UAV[2];
+    vector.w = UAV[3];
+    UAV[16] = vector.x + vector.y + vector.z + vector.w;
+}

tools/clang/test/DXC/min16vector_enabled.hlsl

@@ -0,0 +1,19 @@
+// Validate that the offsets for a min16float vector is correct if 16-bit types are enabled
+// RUN: %dxc -T cs_6_5 -Od -Zi -Qembed_debug -enable-16bit-types %s | FileCheck %s
+
+// CHECK-NOT: {{.*}}!DIDerivedType{{.*}}name: "x"{{.*}}offset: {{.*}}
+// CHECK: {{.*}}!DIDerivedType{{.*}}name: "y"{{.*}}offset: 16{{.*}}
+// CHECK: {{.*}}!DIDerivedType{{.*}}name: "z"{{.*}}offset: 32{{.*}}
+// CHECK: {{.*}}!DIDerivedType{{.*}}name: "w"{{.*}}offset: 48{{.*}}
+
+RWStructuredBuffer<int> UAV : register(u0);
+
+[numthreads(1, 1, 1)] 
+void main() {
+    min16float4 vector;
+    vector.x = UAV[0];
+    vector.y = UAV[1];
+    vector.z = UAV[2];
+    vector.w = UAV[3];
+    UAV[16] = vector.x + vector.y + vector.z + vector.w;
+}

tools/clang/unittests/HLSL/PixDiaTest.cpp

@@ -188,6 +188,8 @@ class PixDiaTest {
   TEST_METHOD(DxcPixDxilDebugInfo_BitFields_Overlap)
   TEST_METHOD(DxcPixDxilDebugInfo_Min16SizesAndOffsets_Enabled)
   TEST_METHOD(DxcPixDxilDebugInfo_Min16SizesAndOffsets_Disabled)
+  TEST_METHOD(DxcPixDxilDebugInfo_Min16VectorOffsets_Enabled)
+  TEST_METHOD(DxcPixDxilDebugInfo_Min16VectorOffsets_Disabled)
   TEST_METHOD(
       DxcPixDxilDebugInfo_VariableScopes_InlinedFunctions_TwiceInlinedFunctions)
   TEST_METHOD(
@@ -661,6 +663,10 @@ class PixDiaTest {
                                 std::array<DWORD, 4> const &memberSizes,
                                 std::vector<const wchar_t *> extraArgs = {
                                     L"-Od"});
+  void RunVectorSizeAndOffsetTestCase(const char *hlsl,
+                                      std::array<DWORD, 4> const &memberOffsets,
+                                      std::vector<const wchar_t *> extraArgs = {
+                                          L"-Od"});
   DebuggerInterfaces
   CompileAndCreateDxcDebug(const char *hlsl, const wchar_t *profile,
                            IDxcIncludeHandler *includer = nullptr,
@@ -3162,6 +3168,83 @@ void main()
   RunSizeAndOffsetTestCase(hlsl, {0, 32, 64, 96}, {16, 16, 16, 16}, {L"-Od"});
 }
 
+TEST_F(PixDiaTest, DxcPixDxilDebugInfo_Min16VectorOffsets_Enabled) {
+  if (m_ver.SkipDxilVersion(1, 5))
+    return;
+
+  const char *hlsl = R"(
+RWStructuredBuffer<int> UAV: register(u0);
+
+[numthreads(1, 1, 1)]
+void main()
+{
+  min16float4 vector;
+  vector.x = UAV[0];
+  vector.y = UAV[1];
+  vector.z = UAV[2];
+  vector.w = UAV[3];
+  UAV[16] = vector.x + vector.y + vector.z + vector.w; //STOP_HERE
+}
+
+
+)";
+  RunVectorSizeAndOffsetTestCase(hlsl, {0, 16, 32, 48},
+                                 {L"-Od", L"-enable-16bit-types"});
+}
+
+TEST_F(PixDiaTest, DxcPixDxilDebugInfo_Min16VectorOffsets_Disabled) {
+  if (m_ver.SkipDxilVersion(1, 5))
+    return;
+
+  const char *hlsl = R"(
+RWStructuredBuffer<int> UAV: register(u0);
+
+[numthreads(1, 1, 1)]
+void main()
+{
+  min16float4 vector;
+  vector.x = UAV[0];
+  vector.y = UAV[1];
+  vector.z = UAV[2];
+  vector.w = UAV[3];
+  UAV[16] = vector.x + vector.y + vector.z + vector.w; //STOP_HERE
+}
+
+
+)";
+  RunVectorSizeAndOffsetTestCase(hlsl, {0, 32, 64, 96});
+}
+void PixDiaTest::RunVectorSizeAndOffsetTestCase(
+    const char *hlsl, std::array<DWORD, 4> const &memberOffsets,
+    std::vector<const wchar_t *> extraArgs) {
+  if (m_ver.SkipDxilVersion(1, 5))
+    return;
+  auto debugInfo =
+      CompileAndCreateDxcDebug(hlsl, L"cs_6_5", nullptr, extraArgs).debugInfo;
+  auto live = GetLiveVariablesAt(hlsl, "STOP_HERE", debugInfo);
+  CComPtr<IDxcPixVariable> variable;
+  VERIFY_SUCCEEDED(live->GetVariableByName(L"vector", &variable));
+  CComPtr<IDxcPixType> type;
+  VERIFY_SUCCEEDED(variable->GetType(&type));
+
+  CComPtr<IDxcPixType> unAliasedType;
+  VERIFY_SUCCEEDED(UnAliasType(type, &unAliasedType));
+  CComPtr<IDxcPixStructType> structType;
+  VERIFY_SUCCEEDED(unAliasedType->QueryInterface(IID_PPV_ARGS(&structType)));
+
+  DWORD fieldCount = 0;
+  VERIFY_SUCCEEDED(structType->GetNumFields(&fieldCount));
+  VERIFY_ARE_EQUAL(fieldCount, 4u);
+
+  for (size_t i = 0; i < memberOffsets.size(); i++) {
+    CComPtr<IDxcPixStructField> field;
+    VERIFY_SUCCEEDED(structType->GetFieldByIndex(i, &field));
+    DWORD offsetInBits = 0;
+    VERIFY_SUCCEEDED(field->GetOffsetInBits(&offsetInBits));
+    VERIFY_ARE_EQUAL(memberOffsets[i], offsetInBits);
+  }
+}
+
 TEST_F(PixDiaTest, DxcPixDxilDebugInfo_SubProgramsInNamespaces) {
   if (m_ver.SkipDxilVersion(1, 2))
     return;