[PATCH] D65368: [DebugInfo] LiveDebugValues: Don't drop fragment information when restoring spills

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 2 inline comments as done.
jmorse added a comment.

> I like the new patch! It looks like you discovered a few more pre-existing bugs though, so I'd suggest to do the following:

Sounds like a plan; I'll open another phab review for patch 1, which should be straight-forwards; the current patch on this review should be sufficient for patch 2; and I'll think about patch 3,

> Once wrinkle: patch 1 or 2 would also need to create undef DEBUG_VALUEs at the spill location for the values we are skipping to terminate the ranges.

Hmn -- is this fully necessary? As it stands, if a spill isn't recognised then the (register) variable lifetime will end in the usual way, when it's clobbered. Recognising spills allows us to extend the variable lifetime past register-clobbering, but it should (AFAIUI) be safe to ignore a spill and let the variable location be clobbered.



================
Comment at: lib/CodeGen/LiveDebugValues.cpp:694
+        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), false, NewReg,
+        DebugInstr->getDebugVariable(), DebugInstr->getDebugExpression());
     VarLoc VL(*NewDebugInstr, LS);
----------------
aprantl wrote:
> So here we are restoring the original expression since DebugInstr points to the pre-spill DBG_VALUE.
Indeed,


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:1121
+        Reg = TRI->getFrameRegister(*DebugInstr->getMF());
+        IsIndirect = true;
+      }
----------------
aprantl wrote:
> Out of curiosity: What happens if IsIndirect was already true? Are we loosing one level of indirection or does this work out?
It looks like the extra indirection is dropped -- line 674 of this patch just passes "true" as the indirect flag for the initial spill, and doesn't inspect the old DBG_VALUEs indirectness.


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

https://reviews.llvm.org/D65368