PIX: Rely on debug info for struct packing (#2883)

Previously, this fragment iterator would attempt to figure out offsets for each struct fragment by itself. This was error-prone and unnecessary: the debug info already has this information.
In particular, the old code didn't handle natural alignment and the possible dead space between fragments that this might create.
Unfortunately the debug info is pretty hard to parse: things with the "member" tag are a few layers of type above the basic type that they actually represent, and contained structs' members have offsets relative to the start of that contained struct, not to the start of the whole struct. Both of these necessitate a recursive approach.
Subsequent work: run these tests with and without -enable-16bit-types and with different optimization levels. The latter is blocked behind bugs in the dbg.value-to-declare pass.

lib/DxilDia/DxcPixLiveVariables_FragmentIterator.cpp

@@ -213,85 +213,91 @@ private:
   std::vector<FragmentSizeAndOffset> m_fragmentLocations;
   unsigned m_currentFragment = 0;
   void CompositeTypeFragmentIterator::DetermineStructMemberSizesAndOffsets(
-    llvm::DIType const*);
+    llvm::DIType const*, uint64_t BaseOffset);
 };
 
-
-void CompositeTypeFragmentIterator::DetermineStructMemberSizesAndOffsets(llvm::DIType const*diType) 
+unsigned SizeIfBaseType(llvm::DIType const* diType)
 {
-  auto AddANewFragment = [=](llvm::DIType const * type)
+  if (auto* BT = llvm::dyn_cast<llvm::DIBasicType>(diType))
   {
-    unsigned size = static_cast<unsigned>(type->getSizeInBits());
-    if (m_fragmentLocations.empty())
-    {
-      m_fragmentLocations.push_back({ size, 0 });
-    }
-    else
-    {
-      unsigned offset = m_fragmentLocations.back().Offset + m_fragmentLocations.back().Size;
-      m_fragmentLocations.push_back({ size, offset });
-    }
-  };
+    return BT->getSizeInBits();
+  }
+  if (auto* DT = llvm::dyn_cast<llvm::DIDerivedType>(diType))
+  {
+    const llvm::DITypeIdentifierMap EmptyMap;
+    return SizeIfBaseType(DT->getBaseType().resolve(EmptyMap));
+  }
+  return 0;
+}
 
-  if (auto* BT = llvm::dyn_cast<llvm::DIBasicType>(diType)) 
+void CompositeTypeFragmentIterator::DetermineStructMemberSizesAndOffsets(llvm::DIType const*diType, uint64_t BaseOffset)
+{
+  if (diType->getTag() == llvm::dwarf::DW_TAG_member)
   {
-    AddANewFragment(BT);
+    BaseOffset += diType->getOffsetInBits();
   }
-  else if (auto* CT = llvm::dyn_cast<llvm::DICompositeType>(diType))
+  unsigned MemberSize = SizeIfBaseType(diType);
+  if (MemberSize != 0)
   {
-    switch (diType->getTag())
-    {
-    case llvm::dwarf::DW_TAG_array_type :
+    m_fragmentLocations.push_back({ MemberSize, static_cast<unsigned>(BaseOffset) });
+  }
+  else
+  {
+    if (auto* CT = llvm::dyn_cast<llvm::DICompositeType>(diType))
     {
-      llvm::DINodeArray elements = CT->getElements();
-      unsigned arraySize = 1;
-      for (auto const& node : elements)
+
+      switch (diType->getTag())
+      {
+      case llvm::dwarf::DW_TAG_array_type:
       {
-        if (llvm::DISubrange* SR = llvm::dyn_cast<llvm::DISubrange>(node))
+        llvm::DINodeArray elements = CT->getElements();
+        unsigned arraySize = 1;
+        for (auto const& node : elements)
         {
-          arraySize *= SR->getCount();
+          if (llvm::DISubrange* SR = llvm::dyn_cast<llvm::DISubrange>(node))
+          {
+            arraySize *= SR->getCount();
+          }
+        }
+        if (arraySize != 0)
+        {
+          const llvm::DITypeIdentifierMap EmptyMap;
+          llvm::DIType* BT = CT->getBaseType().resolve(EmptyMap);
+          unsigned elementSize = static_cast<unsigned>(CT->getSizeInBits()) / arraySize;
+          for (unsigned i = 0; i < arraySize; ++i) {
+            DetermineStructMemberSizesAndOffsets(BT, BaseOffset + i * elementSize);
+          }
         }
       }
-      const llvm::DITypeIdentifierMap EmptyMap;
-      llvm::DIType *BT = CT->getBaseType().resolve(EmptyMap);
-      for (unsigned i = 0; i < arraySize; ++i) {
-        DetermineStructMemberSizesAndOffsets(BT);
-      }
-    }
       break;
-    case llvm::dwarf::DW_TAG_class_type:
-    case llvm::dwarf::DW_TAG_structure_type:
-      for (auto const& node : CT->getElements())
-      {
-        if (llvm::DIType* subType = llvm::dyn_cast<llvm::DIType>(node))
+      case llvm::dwarf::DW_TAG_class_type:
+      case llvm::dwarf::DW_TAG_structure_type:
+        for (auto const& node : CT->getElements())
         {
-          DetermineStructMemberSizesAndOffsets(subType);
+          if (llvm::DIType* subType = llvm::dyn_cast<llvm::DIType>(node))
+          {
+            DetermineStructMemberSizesAndOffsets(subType, BaseOffset /*TODO: plus member offset*/);
+          }
         }
-
+        break;
+      default:
+        diType->dump();
+        break;
       }
-      break;
-    default:
-      diType->dump();
-      break;
     }
-  }
-  else if (auto *DT = llvm::dyn_cast<llvm::DIDerivedType>(diType)) 
-  {
-    const llvm::DITypeIdentifierMap EmptyMap;
-    llvm::DIType *BT = DT->getBaseType().resolve(EmptyMap);
-    DetermineStructMemberSizesAndOffsets(BT);
-  }
-  else
-  {
-    assert(!"Unhandled DIType");
-    throw hlsl::Exception(E_FAIL, "Unhandled DIType");
+    else if (auto* DT = llvm::dyn_cast<llvm::DIDerivedType>(diType))
+    {
+      const llvm::DITypeIdentifierMap EmptyMap;
+      llvm::DIType* BT = DT->getBaseType().resolve(EmptyMap);
+      DetermineStructMemberSizesAndOffsets(BT, BaseOffset);
+    }
   }
 }
 
 CompositeTypeFragmentIterator::CompositeTypeFragmentIterator(
   llvm::DICompositeType* CT)
 {
-  DetermineStructMemberSizesAndOffsets(CT);
+  DetermineStructMemberSizesAndOffsets(CT, 0);
 }
 
 unsigned CompositeTypeFragmentIterator::SizeInBits(unsigned Index) const

tools/clang/unittests/HLSL/PixTest.cpp

@@ -1785,9 +1785,7 @@ PixTest::TestableResults PixTest::TestStructAnnotationCase(const char* hlsl)
           }
           else
           {
-            // Next member has to start where the previous one ended:
-            VERIFY_ARE_EQUAL(iterator->OffsetInBits(memberIndex), startingBit + coveredBits);
-            coveredBits += iterator->SizeInBits(memberIndex);
+            coveredBits = std::max<unsigned int>( coveredBits, iterator->OffsetInBits(memberIndex) + iterator->SizeInBits(memberIndex));
           }
         }
 
@@ -1808,6 +1806,9 @@ PixTest::TestableResults PixTest::TestStructAnnotationCase(const char* hlsl)
         if (auto* ST = llvm::dyn_cast<llvm::StructType>(pAllocaTy))
         {
           uint32_t countOfMembers = CountStructMembers(ST);
+          // memberIndex might be greater, because the fragment iterator also includes contained derived types as
+          // fragments, in addition to the members of that contained derived types. CountStructMembers only counts
+          // the leaf-node types.
           VERIFY_ARE_EQUAL(countOfMembers, memberIndex);
         }
         else if (pAllocaTy->isFloatingPointTy() || pAllocaTy->isIntegerTy())
@@ -1984,7 +1985,8 @@ void main()
   VERIFY_ARE_EQUAL(1, Testables.OffsetAndSizes.size());
   VERIFY_ARE_EQUAL(4, Testables.OffsetAndSizes[0].countOfMembers);
   VERIFY_ARE_EQUAL(0, Testables.OffsetAndSizes[0].offset);
-  VERIFY_ARE_EQUAL(32+64+32+16, Testables.OffsetAndSizes[0].size);
+  // 16+16 to place "thirtytwo" at its natural alignment:
+  VERIFY_ARE_EQUAL(32+16+16+64+32, Testables.OffsetAndSizes[0].size);
 
   VERIFY_ARE_EQUAL(4, Testables.AllocaWrites.size());
   ValidateAllocaWrite(Testables.AllocaWrites, 0, "b1");
@@ -2332,7 +2334,7 @@ void main()
   VERIFY_ARE_EQUAL(0, Testables.OffsetAndSizes[0].offset);
   constexpr uint32_t BigStructBitSize = 64 * 2;
   constexpr uint32_t EmbeddedStructBitSize = 32 * 5;
-  VERIFY_ARE_EQUAL(3 * 32 + EmbeddedStructBitSize + 64 + 16 + BigStructBitSize*2 + 32, Testables.OffsetAndSizes[0].size);
+  VERIFY_ARE_EQUAL(3 * 32 + EmbeddedStructBitSize + 64 + 16 +16/*alignment for next field*/ + BigStructBitSize*2 + 32, Testables.OffsetAndSizes[0].size);
 }