[LiveDebugVariables] Add basic verification

[jayfoad]

Add a basic implementation of verifyAnalysis that just checks that the
analysis does not refer to any SlotIndexes for instructions that have
been deleted. This was useful for diagnosing some SlotIndexes-related
problems caused by #67038.

[llvmbot]

@llvm/pr-subscribers-llvm-regalloc

@llvm/pr-subscribers-debuginfo

Author: Jay Foad (jayfoad)

Changes

Add a basic implementation of verifyAnalysis that just checks the
IntervalMap for consistency. This was useful for diagnosing some
SlotIndexes-related problems caused by #67038.

---

Full diff: https://github.com/llvm/llvm-project/pull/68703.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/LiveDebugVariables.cpp (+16)
• (modified) llvm/lib/CodeGen/LiveDebugVariables.h (+1)

diff --git a/llvm/lib/CodeGen/LiveDebugVariables.cpp b/llvm/lib/CodeGen/LiveDebugVariables.cpp
index 9603c1f01e08569..74546230b36a239 100644
--- a/llvm/lib/CodeGen/LiveDebugVariables.cpp
+++ b/llvm/lib/CodeGen/LiveDebugVariables.cpp
@@ -491,6 +491,12 @@ class UserValue {
   /// Return DebugLoc of this UserValue.
   const DebugLoc &getDebugLoc() { return dl; }
 
+  void verify() const {
+    // Verify consistency of the intervals in locInts.
+    for (auto I = locInts.begin(), E = locInts.end(); I != E; ++I)
+      assert(LocMap::KeyTraits::nonEmpty(I.start(), I.stop()));
+  }
+
   void print(raw_ostream &, const TargetRegisterInfo *);
 };
 
@@ -654,6 +660,11 @@ class LDVImpl {
     ModifiedMF = false;
   }
 
+  void verify() const {
+    for (auto [DV, UV] : userVarMap)
+      UV->verify();
+  }
+
   /// Map virtual register to an equivalence class.
   void mapVirtReg(Register VirtReg, UserValue *EC);
 
@@ -1319,6 +1330,11 @@ void LiveDebugVariables::releaseMemory() {
     static_cast<LDVImpl*>(pImpl)->clear();
 }
 
+void LiveDebugVariables::verifyAnalysis() const {
+  if (pImpl)
+    static_cast<LDVImpl*>(pImpl)->verify();
+}
+
 LiveDebugVariables::~LiveDebugVariables() {
   if (pImpl)
     delete static_cast<LDVImpl*>(pImpl);
diff --git a/llvm/lib/CodeGen/LiveDebugVariables.h b/llvm/lib/CodeGen/LiveDebugVariables.h
index 9998ce9e8dad861..c99f11c8565a819 100644
--- a/llvm/lib/CodeGen/LiveDebugVariables.h
+++ b/llvm/lib/CodeGen/LiveDebugVariables.h
@@ -56,6 +56,7 @@ class LLVM_LIBRARY_VISIBILITY LiveDebugVariables : public MachineFunctionPass {
   bool runOnMachineFunction(MachineFunction &) override;
   void releaseMemory() override;
   void getAnalysisUsage(AnalysisUsage &) const override;
+  void verifyAnalysis() const override;
 
   MachineFunctionProperties getSetProperties() const override {
     return MachineFunctionProperties().set(

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 600e38b11ae79b0c0cd10416b2a7a78f654cf5b2 cc0a5f4f6016b31e14e06363f42e168d2060a286 -- llvm/include/llvm/CodeGen/SlotIndexes.h llvm/lib/CodeGen/LiveDebugVariables.cpp llvm/lib/CodeGen/LiveDebugVariables.h llvm/lib/CodeGen/SlotIndexes.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/CodeGen/SlotIndexes.h b/llvm/include/llvm/CodeGen/SlotIndexes.h
index 2b437a0649aa..75fae0de04e3 100644
--- a/llvm/include/llvm/CodeGen/SlotIndexes.h
+++ b/llvm/include/llvm/CodeGen/SlotIndexes.h
@@ -57,26 +57,20 @@ class raw_ostream;
     unsigned index;
 
   public:
-    IndexListEntry(MachineInstr *mi, unsigned index) : mi(mi, 0), index(index) {
-    }
+    IndexListEntry(MachineInstr *mi, unsigned index)
+        : mi(mi, 0), index(index) {}
 
-    MachineInstr* getInstr() const { return mi.getPointer(); }
-    void setInstr(MachineInstr *mi) {
-      this->mi.setPointer(mi);
-    }
+    MachineInstr *getInstr() const { return mi.getPointer(); }
+    void setInstr(MachineInstr *mi) { this->mi.setPointer(mi); }
 
     unsigned getIndex() const { return index; }
     void setIndex(unsigned index) {
       this->index = index;
     }
 
-    void setPoison() {
-      mi.setInt(PoisonVal);
-    }
+    void setPoison() { mi.setInt(PoisonVal); }
 
-    bool isPoisoned() const {
-      return mi.getInt();
-    }
+    bool isPoisoned() const { return mi.getInt(); }
   };
 
   template <>
@@ -305,9 +299,7 @@ class raw_ostream;
       return SlotIndex(&*--listEntry()->getIterator(), getSlot());
     }
 
-    bool isPoisoned() const {
-      return listEntry()->isPoisoned();
-    }
+    bool isPoisoned() const { return listEntry()->isPoisoned(); }
   };
 
   inline raw_ostream& operator<<(raw_ostream &os, SlotIndex li) {

[jmorse]

In principle this LGTM, although it's been a long time since I've looked at LiveDebugVariables. As far as I recall, we use setUnchecked to do manual maintenance of ranges but they should all coalesce in the end. And it doesn't make sense for there to be a zero-length range of variable location.

This does involve iterating over every range of every variable in the function, which might have compile-time performance implications. Is it worth putting this behind an EXPENSIVE_CHECKS ifdef? (I haven't followed the SlotIndex related problems you've mentioned, I don't know whether it needs running on every compilation)

[jayfoad]

In principle this LGTM, although it's been a long time since I've looked at LiveDebugVariables. As far as I recall, we use setUnchecked to do manual maintenance of ranges but they should all coalesce in the end. And it doesn't make sense for there to be a zero-length range of variable location.

Stepping back a bit: I am trying to diagnose problems where the cached LiveDebugVariables analysis info refers to SlotIndexes for instructions that have been deleted. (Possibly this is caused by RegAlloc not updating LDV properly - I am still investigating that aspect of it.) I am pretty sure that this is not allowed, although currently it seems to be harmless because SlotIndexes does not actually delete or reuse those slots. #67038 tried to change SlotIndexes so that it no longer maintains these deleted slots and they can end up containing garbage values. The "zero-length range" issue that I check for here is just one possible consequence of depending on those garbage values.

It would be better if I could directly query SlotIndexes to ask "is this slot still live?" (under #ifndef NDEBUG). I'll work on implementing something like that.

This does involve iterating over every range of every variable in the function, which might have compile-time performance implications. Is it worth putting this behind an EXPENSIVE_CHECKS ifdef? (I haven't followed the SlotIndex related problems you've mentioned, I don't know whether it needs running on every compilation)

I think it is generally acceptable performance-wise for verifyAnalysis to make a single pass over all information cached by an analysis.

[jayfoad]

It would be better if I could directly query SlotIndexes to ask "is this slot still live?" (under #ifndef NDEBUG). I'll work on implementing something like that.

I have reworked the patch along those lines. It currently causes some lit tests failures in my build. I would suggest that these are all pre-existing problems that really ought to be fixed.

Failed Tests (6):
  LLVM :: CodeGen/Thumb2/pr52817.ll
  LLVM :: DebugInfo/ARM/PR26163.ll
  LLVM :: DebugInfo/COFF/fpo-csrs.ll
  LLVM :: DebugInfo/X86/pr34545.ll
  LLVM :: DebugInfo/X86/spill-indirect-nrvo.ll
  LLVM :: tools/llvm-dwarfdump/X86/LTO_CCU_zero_loc_cov.ll

[jmorse]

This fell off my radar, sorry. Thanks for the background on what's going on here, FTR, this patch LGTM. Note that there are two flavours of debug-info format in the CodeGen pipeline and the records you're verifying only addresses one of them, there's also a collection that contains SlotIndexes in StashedDebugInstrs and PHIValToPos that you might want to verify too.

I had a look at one of the failures (spill-indirect-nrvo.ll), and it would appear that:

• We split a virtual register location with a COPY,
• LiveDebugVariables updates it's mappings to record the new location at the position of the copy, with vreg %7
• InlineSpiller::spill comes along and spills the value, making the COPY a dead definition (apparently),
• The mapping of slot-index to vreg %7 continues on in LiveDebugVariables,
• When VirtRegRewriter comes along and emits DBG_VALUE instructions, in UserValue::rewriteLocations, vreg %7 which isn't otherwise defined has a stack location looked up by VRM.getStackSlot. As it's the same location as earlier DBG_VALUEs, they get coalesced together (I think) and the dead slot index doesn't get used.

This feels like an error where, by chance, the right thing happens most of the time. I've got various feelings on solutions:

• We could rewrite all the location-mappings at the end of regalloc to ensure illegal slotindexes get coalesced away early,
• We could explicitly signal from the regalloc code that a definition is being deleted.

As far as I'm aware LiveDebugVariables doesn't have any concept of "this assignment is just going to completely go away", LiveRangeEdit::eliminateDeadDef would have to be updating LiveDebugVariables to let it know. I'm not sure how we'd then update the variable location data in that circumstance.

CC @felipepiovezan as we won't be seeing this on X86 as it uses the other CodeGen debug-info format, but it could be an issue that arm-like systems see.

[jayfoad]

Thanks for the investigation! I'm busy with other things at the moment but will come back to this when I have some time.

[jayfoad]

jayfoad closed this by deleting the head repository 4 hours ago

Sorry. Recreated as #79846.