[PATCH] D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations

Thu Jan 3 03:28:19 PST 2019

jmorse added a comment.

Thanks for the review,

In D56151#1343002 <https://reviews.llvm.org/D56151#1343002>, @vsk wrote:

> As you pointed out in llvm.org/PR40010, we do have a general problem of leaving around DBG_VALUE instructions which should be dead/undef but aren't. Do you think it would be possible/worthwhile to diagnose this issue in MachineVerifier?

IMHO that's a definite yes, eventually. It's non-trivial for a human to see where liveness starts and ends for DBG_VALUEs, but a significant liability for optimisations as they can't "see" the DBG_VALUEs they're messing with. A truly dead DBG_VALUE will be dropped anyway, because it has no meaning, so it's not a useful feature to keep.

I'm not familiar with the machine verifier, but it appears to check liveness for things like kill flags, and just ignores debug users, so enabling checking of DBG_VALUEs should be easy to implement.

(I'm preparing a patch that recovers dead DBG_VALUEs from still-live copies, which would have to change if we made dead DBG_VALUEs illegal, but that's a different matter).

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1895
+  SmallVector<MachineInstr *, 4> DbgValuesToChange;
+  for (MachineRegisterInfo::reg_instr_iterator
+           I = MRI->reg_instr_begin(CP.getSrcReg()),
----------------
vsk wrote:
> Using MachineRegisterInfo::reg_instructions might aid readability here. Any reason not to?
No reason (lack of familiarity), I'll switch to that,

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1901
+    if (UseMI->isDebugValue() && UseMI->getOperand(0).isReg() &&
+        TargetRegisterInfo::isVirtualRegister(CP.getDstReg()) &&
+        mergingChangesDbgValue(UseMI, LIS->getInterval(CP.getDstReg())))
----------------
vsk wrote:
> Could you explain why it's sufficient to fix up coalescer pairs in which the destination is a vreg?
Hmmm. I've been operating on the assumption that the coalescer only operates on vregs, but I now see that isn't necessarily the case... I'll investigate this one.

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

https://reviews.llvm.org/D56151