[PATCH] D83890: [DebugInfo] Process DBG_VALUE_LIST in LiveDebugValues

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

I'm going to tentatively "LGTM" this --and I've read through it a couple of times and think it works, it'd be great if someone else could chip in. There are various things that aren't ideal, but that stems from having to track things across multiple entries. It's encouraging that @vsk's benchmark didn't show a regression. I've added some minor comments inline.

I'd highly recommend giving the "location zero" index a fixed number, like kEntryValueBackupLocation, to identify all the parts of LiveDebugValues that rely on it.

There's a small amount of code to cope with scenarios where a DBG_VALUE_LIST has duplicate operands; if we're going to support this, there'll need to be a test that this is properly handled.

Could I also ask for a test featuring a DBG_VALUE_LIST with a $noreg operand, just to check that doesn't propagate any further.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:210
+// range corresponding to a Reg, SpillLoc, or EntryValue type will still be a
+// true set, and the range Location=0 is the set of all VarLocs.
 using VarLocSet = CoalescingBitVector<uint64_t>;
----------------
I'm not sure what "true set" means here


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:320-325
+    enum class MachineLocKind {
       InvalidKind = 0,
       RegisterKind,
       SpillLocKind,
-      ImmediateKind,
+      ImmediateKind
+    };
----------------
Comment indicating these are only meaningful for NonEntryValueKind VarLocs?




================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:387
+    /// conjunction with Expr. Initially populated with MI's debug operands,
+    /// but maybe modified independently afterwards.
+    SmallVector<MachineLoc, 8> Locs;
----------------
nit: I would say transformed, as VarLocs themselves should be constant.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:389-391
+    /// Used to map the index of each location in Locs back to the index of its
+    /// original debug operand in MI.
+    SmallVector<unsigned, 8> OrigLocMap;
----------------
As I understand it, this is to cope with duplicate locations from a MachineInstr, yes? If so, that should go in the comment.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:704-705
     bool operator==(const VarLoc &Other) const {
-      if (Kind != Other.Kind || !(Var == Other.Var) || Expr != Other.Expr)
-        return false;
-
-      switch (Kind) {
-      case SpillLocKind:
-        return Loc.SpillLocation == Other.Loc.SpillLocation;
-      case RegisterKind:
-      case ImmediateKind:
-      case EntryValueKind:
-      case EntryValueBackupKind:
-      case EntryValueCopyBackupKind:
-        return Loc.Hash == Other.Loc.Hash;
-      default:
-        llvm_unreachable("Invalid kind");
-      }
+      return EVKind == Other.EVKind && Var == Other.Var && Expr == Other.Expr &&
+             Locs == Other.Locs;
     }
----------------
std::tie for consistency?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:715
 
+  using VarVec = SmallVector<VarLoc, 32>;
+
----------------
I worry that this will hit "unused type / name" warnings in later compilers, can we guard it with NDEBUG, like its uses?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:807
+  void getUsedRegs(const VarLocSet &CollectFrom,
+                   SmallVectorImpl<uint32_t> &UsedRegs) const;
+
----------------
Register rather than uint32_t?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:848
+    void erase(const VarLocsInRange &KillSet, const VarLocMap &VarLocIDs,
+               LocIndex::u32_location_t Location = 0);
 
----------------
IMHO having a default location-bucket here is undesirable: the KillSet is meaningless without it, and it raises the possibility of accidentally clearing VarLocs from location-bucket zero, when a different location-bucket should have been used.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1124
+      LocIndices LI = VarLocIDs.getAllIndices(VL);
+      // For now, the back index is always Loc=0
+      Collected.insert(LI.back().Index);
----------------
assertion for that? Strikes me as fragile otherwise.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1228-1229
   const MachineOperand *SrcRegOp, *DestRegOp;
-  if (I != MI.getParent()->rend()) {
-    // TODO: Try to keep tracking of an entry value if we encounter a propagated
-    // DBG_VALUE describing the copy of the entry value. (Propagated entry value
-    // does not indicate the parameter modification.)
-    auto DestSrc = TII->isCopyInstr(*I);
-    if (!DestSrc)
-      return true;
+  if (I == MI.getParent()->rend())
+    return true;
 
----------------
This and nearby reformatting looks to be a no-op refactor, yes? IMO best to remove it, or add it as a follow-up, to avoid further review / audit burden.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1307
 
+// This should be removed later, doesn't fit the new design.
+void VarLocBasedLDV::collectVarLocsForReg(SmallVectorImpl<VarLoc> &Collected,
----------------
Is now "later"? If this needs to stay it, better to document it and what its limitation is (reduces bus factor)


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1335-1336
 
-  for (uint64_t ID : KillSet) {
-    LocIndex Idx = LocIndex::fromRawInteger(ID);
+  for (LocIndex::u32_index_t ID : KillSet) {
+    LocIndex Idx = LocIndex(0, ID);
     const VarLoc &VL = VarLocIDs[Idx];
----------------
Explanation of the magic zero would help, "Iterate over all VarLocs -- they will all have an entry at location zero"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83890/new/

https://reviews.llvm.org/D83890