[PATCH] D54717: [WIP][DebugInfo] Use VReg users to find DBG_VALUEs for a value

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
Herald added a subscriber: llvm-commits.

This is a work-in-progress related to PR38754 [0], uploaded for visibility rather than review.

This patch is ugly because the behaviour exposed by the patch is ugly. If you disable placeDbgValues (topic of [0]), the currently semi-legitimate assumption of collectDebugValues (that DBG_VALUEs always follow the values definition) is eliminated, meaning DBG_VALUEs of a vreg can be in any BB in the function.

On the whole this is what we want IMHO, because that might be the debug-model of the program. However, we get a crash in MachineSink if it tries to sink a DBG_VALUE in front of the value-definition (maybe an un-related use-before-def bug), and a variety of infinite loops if DBG_VALUEs are sunk out of other BBs.

These are all matters that can be considered and fixed, they've just never been problems because placeDbgValues has squashed any kind of complicated DBG_VALUE layout. I've uploaded this patch because it allows clang/llvm to compile itself, might be a starting point for making other improvements. I've also only tested it with https://reviews.llvm.org/D54716 reducing the amount of code that uses collectDebugValues.

[0] https://bugs.llvm.org/show_bug.cgi?id=38754


Repository:
  rL LLVM

https://reviews.llvm.org/D54717

Files:
  lib/CodeGen/MachineInstr.cpp
  lib/CodeGen/MachineSink.cpp


Index: lib/CodeGen/MachineSink.cpp
===================================================================
--- lib/CodeGen/MachineSink.cpp
+++ lib/CodeGen/MachineSink.cpp
@@ -367,10 +367,13 @@
     MachineInstr &MI = *I;  // The instruction to sink.
 
     // Predecrement I (if it's not begin) so that it isn't invalidated by
-    // sinking.
-    ProcessedBegin = I == MBB.begin();
-    if (!ProcessedBegin)
-      --I;
+    // sinking. Continue to decrement it past any debug insns, which might
+    // be sunk out of this BB.
+    do {
+      ProcessedBegin = I == MBB.begin();
+      if (!ProcessedBegin)
+        --I;
+    } while (!ProcessedBegin && I->isDebugInstr());
 
     if (MI.isDebugInstr())
       continue;
Index: lib/CodeGen/MachineInstr.cpp
===================================================================
--- lib/CodeGen/MachineInstr.cpp
+++ lib/CodeGen/MachineInstr.cpp
@@ -2080,14 +2080,28 @@
   if (!MI.getOperand(0).isReg())
     return;
 
-  MachineBasicBlock::iterator DI = MI; ++DI;
-  for (MachineBasicBlock::iterator DE = MI.getParent()->end();
-       DI != DE; ++DI) {
+  auto *MRI = getRegInfo();
+#ifndef NDEBUG
+  // There are no guarantees on DBG_VALUE availability and ordering after
+  // regalloc, they must be sought manually.
+  auto *TRI = MRI->getTargetRegisterInfo();
+  assert(!TRI->isPhysicalRegister(MI.getOperand(0).getReg()));
+#endif
+
+  for (auto &MO : MRI->use_operands(MI.getOperand(0).getReg())) {
+    auto *DI = MO.getParent();
     if (!DI->isDebugValue())
-      return;
+      continue;
+    // XXX XXX XXX numerous things explode if we "update" or move DBG_VALUEs
+    // from outside the current BB. These were previously ignored anyway by
+    // the prior implementation of collectDebugValues, so technically this
+    // isn't a regression...
+    if (DI->getParent() != getParent())
+      continue;
     if (DI->getOperand(0).isReg() &&
-        DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
-      DbgValues.push_back(&*DI);
+        DI->getOperand(0).getReg() == MI.getOperand(0).getReg()) {
+      DbgValues.push_back(DI);
+    }
   }
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54717.174658.patch
Type: text/x-patch
Size: 2119 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181119/8ee95786/attachment.bin>