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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 5 inline comments as done.
jmorse added inline comments.


================
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();
----------------
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?


================
Comment at: lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:282
 
       if (MI.isDebugValue()) {
         assert(MI.getNumOperands() > 1 && "Invalid DBG_VALUE instruction!");
----------------
aprantl wrote:
> For readability it might be a good idea to move the short cases to the top, with a `continue`, then we have more horizontal space for the meat of the function.
I've shuffled the shorter cases to the top and added a continue; alas, this makes for a very different patch to the previous one.

For the RegsToClobber vector I've upped the size to 32 elements, on the principle that stack allocation is more or less free, and we're (broadly) a leaf function.


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

https://reviews.llvm.org/D61940