Fix debug info offsets for vectors with 16-bit types (microsoft#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.

Author: gracezhang72

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;