[PATCH] D61940: [DebugInfo] Don't always extend variable locations when the reg location is unchanging

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

Is the intention to change the behavior for prologues as well here? The description mentions that we do a different analysis for the epilogues. But with this patch we no longer ignore register clobbering for "FrameSetup" (collectChangingRegs used to ignore FrameSetup code).

Maybe that is a good thing (just not obvious when reading the description that this patch also treats prologues differently).



================
Comment at: lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:257
+            else if (MO.getReg() != FrameReg ||
+                     !MI.getFlag(MachineInstr::FrameDestroy)) {
               for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
----------------
jmorse wrote:
> bjope wrote:
> > The old solution using getFirstEpilogueInst was a little bit weird so it is nice to get rid of that. However, if I remember correctly FrameDestroy isn't used by all in-tree targets (and this change is not fully backwards compatible for OOT targets either).
> > 
> > Maybe that shouldn't stop us. But just something that should be taken into consideration (maybe we should make sure in-tree targets are using FrameDestroy consistently and make it obvious in commit msg or something that we now rely on FrameDestroy here).
> > It could even be possible to swap into relying on FrameDestory in a separate patch.
> > 
> Ah, I read about this in the history, but didn't think about it too deeply. The interaction between OOT backends and the rest of LLVM is largely opaque to me, however IIUC, the impact of this behaviour change is that stack-locations will end prematurely in the epilogue, and they won't get as nice DWARF-expressions for such locations. Would this be sufficiently minor for it to go ahead?
> 
> I'll examine the in-tree backends to check that they're using FrameDestroy correctly.
> 
> Splitting this patch would likely require marking some tests XFail in the meantime, as many single-location values wouldn't be generated. Perhaps that's desirable, as OOT backends could then revert the FrameDestroy patch and have the correct tests marked XFail?
> Ah, I read about this in the history, but didn't think about it too deeply. The interaction between OOT backends and the rest of LLVM is largely opaque to me, however IIUC, the impact of this behaviour change is that stack-locations will end prematurely in the epilogue, and they won't get as nice DWARF-expressions for such locations. Would this be sufficiently minor for it to go ahead?

Ok, makes sense.




================
Comment at: lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:277
+          // If this is a register def operand, it may end a debug value
+          // range. Ignore frame-register defs in the epilogue, we expect
+          // debuggers to understand that stack-locations are destroyed on
----------------
Comment here is related to D62314 (that is a child). Maybe D62314 should be a parent instead (unless that complicates things even more for you)?


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

https://reviews.llvm.org/D61940