[PATCH] D57271: [DebugInfo] Handle restore instructions in LiveDebugValues

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/CodeGen/MachineInstr.h:1403
+  /// Return a valid size if the instruction is a spill instruction.
+  Optional<unsigned> isSpillInstruction(const TargetInstrInfo *TII) const;
+
----------------
wolfgangp wrote:
> aprantl wrote:
> > aprantl wrote:
> > > Might be better to call this getSpillInstructionSize if it isn't returning a bool. Naming multi-purpose functions i always awkward :-)
> > Either:
> > 
> > `Optional<unsigned> getSpillInstruction(const TargetInstrInfo *TII) const;`
> > 
> > which makes the uses in AsmPrinter a bit odd.
> > 
> > Or one of
> > `bool  isSpillInstruction(const TargetInstrInfo *TII, unsigned &Size) const;`
> > `bool  isSpillInstruction(const TargetInstrInfo *TII, unsigned *Size = nullptr) const;`
> > 
> > which looks less elegant.
> How about
> 
> Optional<unsigned> getSpillSize(const TargetInstrInfo *TII) const;
> 
> etc.
> 
> This conveys retrieving the size as the main purpose of the function (we're not interested in the size of the instruction, only the size of the spilled/restored entity). If the instruction is not a spill/restore no meaningful size is returned (as opposed to size 0, which means we found a valid restore/spill but couldn't figure out what it's (re)storing).
Looks good!


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:543
 
-  // To identify a spill instruction, use the same criteria as in AsmPrinter.
-  if (!((TII->isStoreToStackSlotPostFE(MI, FI) &&
-         FrameInfo.isSpillSlotObjectIndex(FI)) ||
-        (TII->hasStoreToStackSlot(MI, Accesses) &&
-         llvm::any_of(Accesses, [&FrameInfo](const MachineMemOperand *MMO) {
-           return FrameInfo.isSpillSlotObjectIndex(
-               cast<FixedStackPseudoSourceValue>(MMO->getPseudoValue())
-                   ->getFrameIndex());
-         }))))
+  if (!MI.getSpillSize(TII) && !MI.getFoldedSpillSize(TII))
     return false;
----------------
I'd add something like: `// This is not a spill instruction.`


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:633
+      Match = true;
+    }
+    if (TKind == TransferKind::TransferRestore &&
----------------
`else if`


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:639
+      Match = true;
     }
+    if (Match)
----------------
`else continue;`
then we don't need `Match` any more.


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

https://reviews.llvm.org/D57271