[WebAssembly] Redesign DebugValueManager

The current `DebugValueManager`, which is mostly used in `RegStackify`,
simply sinks `DBG_VALUE`s along when a def instruction sinks.
(`RegStackify` only does sinks; it doesn't do hoists.)

But this simple strategy can result in incorrect combinations of
variables' values which would have not been possible in the original
program. In this case, LLVM's policy is to make the value unavailable,
so they will be shown as 'optimized out', rather than showing inaccurate
debug info. Especially, when an instruction sinks, its original
`DBG_VALUE` should be set to undef. This is well illustrated in the
third example in
https://llvm.org/docs/SourceLevelDebugging.html#instruction-scheduling.

This CL rewrites `DebugValueManager` with this principle in mind. When
sinking an instruction, it sinks its eligible `DBG_VALUE`s with it, but
also leaves undef `DBG_VALUE`s in the original place to make those
variables' values undefined.

Also, unlike the current version, we sink only an eligible subset of
`DBG_VALUE`s with a def instruction. See comments in the code for
details.

In case of cloning, because the original def is still there, we don't
set its `DBG_VALUE`s to undef. But we clone only an eligible subset of
`DBG_VALUE`s here as well.

One consequence of this change is that now we do sinking and cloning of
the def instruction itself within the `DebugValueManager`'s `sink` and
`clone` methods. This is necessary because the `DebugValueManager` needs
to know the original def's location before sinking and cloning in order
to scan other interfering `DBG_VALUE`s between the original def and the
insertion point. If we want to separate these two, we need to call
`DebugValueManager`'s `sink` and `clone` methods //before//
sinking/cloning the def instruction, which I don't think is a good
design alternative either, because the user of this class needs to pay
extra attention when using it.

Because this change is fixing the existing inaccuracy of the current
debug info, this reduces the variable info coverage in debug info, but
not by a large margin. In Emscripten core benchmarks compiled with
`-O1`, the coverage goes from 56.6% down to 55.2%, which I doubt will be
a noticeable drop. The compilation time doesn't have any meaningful
difference either with this change.

Reviewed By: dschuff

Differential Revision: https://reviews.llvm.org/D146744

1 files changed, 33 insertions, 32 deletions

diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index 4b24f7f..2e0df3c 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -278,12 +278,13 @@ static MachineInstr *getVRegDef(unsigned Reg, const MachineInstr *Insert,
 }
 
 // Test whether Reg, as defined at Def, has exactly one use. This is a
-// generalization of MachineRegisterInfo::hasOneUse that uses LiveIntervals
-// to handle complex cases.
-static bool hasOneUse(unsigned Reg, MachineInstr *Def, MachineRegisterInfo &MRI,
-                      MachineDominatorTree &MDT, LiveIntervals &LIS) {
+// generalization of MachineRegisterInfo::hasOneNonDBGUse that uses
+// LiveIntervals to handle complex cases.
+static bool hasOneNonDBGUse(unsigned Reg, MachineInstr *Def,
+                            MachineRegisterInfo &MRI, MachineDominatorTree &MDT,
+                            LiveIntervals &LIS) {
   // Most registers are in SSA form here so we try a quick MRI query first.
-  if (MRI.hasOneUse(Reg))
+  if (MRI.hasOneNonDBGUse(Reg))
     return true;
 
   bool HasOne = false;
@@ -525,11 +526,10 @@ static MachineInstr *moveForSingleUse(unsigned Reg, MachineOperand &Op,
   LLVM_DEBUG(dbgs() << "Move for single use: "; Def->dump());
 
   WebAssemblyDebugValueManager DefDIs(Def);
-  MBB.splice(Insert, &MBB, Def);
-  DefDIs.move(Insert);
+  DefDIs.sink(Insert);
   LIS.handleMove(*Def);
 
-  if (MRI.hasOneDef(Reg) && MRI.hasOneUse(Reg)) {
+  if (MRI.hasOneDef(Reg) && MRI.hasOneNonDBGUse(Reg)) {
     // No one else is using this register for anything so we can just stackify
     // it in place.
     MFI.stackifyVReg(MRI, Reg);
@@ -537,8 +537,8 @@ static MachineInstr *moveForSingleUse(unsigned Reg, MachineOperand &Op,
     // The register may have unrelated uses or defs; create a new register for
     // just our one def and use so that we can stackify it.
     Register NewReg = MRI.createVirtualRegister(MRI.getRegClass(Reg));
-    Def->getOperand(0).setReg(NewReg);
     Op.setReg(NewReg);
+    DefDIs.updateReg(NewReg);
 
     // Tell LiveIntervals about the new register.
     LIS.createAndComputeVirtRegInterval(NewReg);
@@ -551,8 +551,6 @@ static MachineInstr *moveForSingleUse(unsigned Reg, MachineOperand &Op,
 
     MFI.stackifyVReg(MRI, NewReg);
 
-    DefDIs.updateReg(NewReg);
-
     LLVM_DEBUG(dbgs() << " - Replaced register: "; Def->dump());
   }
 
@@ -560,6 +558,13 @@ static MachineInstr *moveForSingleUse(unsigned Reg, MachineOperand &Op,
   return Def;
 }
 
+static MachineInstr *getPrevNonDebugInst(MachineInstr *MI) {
+  for (auto *I = MI->getPrevNode(); I; I = I->getPrevNode())
+    if (!I->isDebugInstr())
+      return I;
+  return nullptr;
+}
+
 /// A trivially cloneable instruction; clone it and nest the new copy with the
 /// current instruction.
 static MachineInstr *rematerializeCheapDef(
@@ -573,9 +578,10 @@ static MachineInstr *rematerializeCheapDef(
   WebAssemblyDebugValueManager DefDIs(&Def);
 
   Register NewReg = MRI.createVirtualRegister(MRI.getRegClass(Reg));
-  TII->reMaterialize(MBB, Insert, NewReg, 0, Def, *TRI);
+  DefDIs.cloneSink(&*Insert, NewReg);
   Op.setReg(NewReg);
-  MachineInstr *Clone = &*std::prev(Insert);
+  MachineInstr *Clone = getPrevNonDebugInst(&*Insert);
+  assert(Clone);
   LIS.InsertMachineInstrInMaps(*Clone);
   LIS.createAndComputeVirtRegInterval(NewReg);
   MFI.stackifyVReg(MRI, NewReg);
@@ -592,19 +598,13 @@ static MachineInstr *rematerializeCheapDef(
   }
 
   // If that was the last use of the original, delete the original.
-  // Move or clone corresponding DBG_VALUEs to the 'Insert' location.
   if (IsDead) {
     LLVM_DEBUG(dbgs() << " - Deleting original\n");
     SlotIndex Idx = LIS.getInstructionIndex(Def).getRegSlot();
     LIS.removePhysRegDefAt(MCRegister::from(WebAssembly::ARGUMENTS), Idx);
     LIS.removeInterval(Reg);
     LIS.RemoveMachineInstrFromMaps(Def);
-    Def.eraseFromParent();
-
-    DefDIs.move(&*Insert);
-    DefDIs.updateReg(NewReg);
-  } else {
-    DefDIs.clone(&*Insert, NewReg);
+    DefDIs.removeDef();
   }
 
   return Clone;
@@ -636,28 +636,26 @@ static MachineInstr *moveAndTeeForMultiUse(
     MachineRegisterInfo &MRI, const WebAssemblyInstrInfo *TII) {
   LLVM_DEBUG(dbgs() << "Move and tee for multi-use:"; Def->dump());
 
-  WebAssemblyDebugValueManager DefDIs(Def);
+  const auto *RegClass = MRI.getRegClass(Reg);
+  Register TeeReg = MRI.createVirtualRegister(RegClass);
+  Register DefReg = MRI.createVirtualRegister(RegClass);
 
   // Move Def into place.
-  MBB.splice(Insert, &MBB, Def);
+  WebAssemblyDebugValueManager DefDIs(Def);
+  DefDIs.sink(Insert);
   LIS.handleMove(*Def);
 
   // Create the Tee and attach the registers.
-  const auto *RegClass = MRI.getRegClass(Reg);
-  Register TeeReg = MRI.createVirtualRegister(RegClass);
-  Register DefReg = MRI.createVirtualRegister(RegClass);
   MachineOperand &DefMO = Def->getOperand(0);
   MachineInstr *Tee = BuildMI(MBB, Insert, Insert->getDebugLoc(),
                               TII->get(getTeeOpcode(RegClass)), TeeReg)
                           .addReg(Reg, RegState::Define)
                           .addReg(DefReg, getUndefRegState(DefMO.isDead()));
   Op.setReg(TeeReg);
-  DefMO.setReg(DefReg);
+  DefDIs.updateReg(DefReg);
   SlotIndex TeeIdx = LIS.InsertMachineInstrInMaps(*Tee).getRegSlot();
   SlotIndex DefIdx = LIS.getInstructionIndex(*Def).getRegSlot();
 
-  DefDIs.move(Insert);
-
   // Tell LiveIntervals we moved the original vreg def from Def to Tee.
   LiveInterval &LI = LIS.getInterval(Reg);
   LiveInterval::iterator I = LI.FindSegmentContaining(DefIdx);
@@ -674,8 +672,11 @@ static MachineInstr *moveAndTeeForMultiUse(
   imposeStackOrdering(Def);
   imposeStackOrdering(Tee);
 
-  DefDIs.clone(Tee, DefReg);
-  DefDIs.clone(Insert, TeeReg);
+  // Even though 'TeeReg, Reg = TEE ...', has two defs, we don't need to clone
+  // DBG_VALUEs for both of them, given that the latter will cancel the former
+  // anyway. Here we only clone DBG_VALUEs for TeeReg, which will be converted
+  // to a local index in ExplicitLocals pass.
+  DefDIs.cloneSink(Insert, TeeReg, /* CloneDef */ false);
 
   LLVM_DEBUG(dbgs() << " - Replaced register: "; Def->dump());
   LLVM_DEBUG(dbgs() << " - Tee instruction: "; Tee->dump());
@@ -876,7 +877,7 @@ bool WebAssemblyRegStackify::runOnMachineFunction(MachineFunction &MF) {
         bool SameBlock = DefI->getParent() == &MBB;
         bool CanMove = SameBlock && isSafeToMove(Def, &Use, Insert, MFI, MRI) &&
                        !TreeWalker.isOnStack(Reg);
-        if (CanMove && hasOneUse(Reg, DefI, MRI, MDT, LIS)) {
+        if (CanMove && hasOneNonDBGUse(Reg, DefI, MRI, MDT, LIS)) {
           Insert = moveForSingleUse(Reg, Use, DefI, MBB, Insert, LIS, MFI, MRI);
 
           // If we are removing the frame base reg completely, remove the debug
@@ -913,7 +914,7 @@ bool WebAssemblyRegStackify::runOnMachineFunction(MachineFunction &MF) {
           Register DefReg = SubsequentDef->getReg();
           Register UseReg = SubsequentUse->getReg();
           // TODO: This single-use restriction could be relaxed by using tees
-          if (DefReg != UseReg || !MRI.hasOneUse(DefReg))
+          if (DefReg != UseReg || !MRI.hasOneNonDBGUse(DefReg))
             break;
           MFI.stackifyVReg(MRI, DefReg);
           ++SubsequentDef;