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

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp marked an inline comment as done.
wolfgangp 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;
+
----------------
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).


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

https://reviews.llvm.org/D57271