Page MenuHomePhabricator

[DebugInfo] Handle restore instructions in LiveDebugValues
ClosedPublic

Authored by wolfgangp on Jan 25 2019, 5:16 PM.

Details

Summary

We recognize spill instructions in LiveDebugValues, i.e. we create DBG_VALUE instructions for variables that are spilled with the appropriate spill locations. We do not, however, recognize the corresponding restore instructions, which can cause large gaps in location information. For example: given a basic block BB with 2 or more predecessors, a variable is live in a register at the exit of the first predecessor, but has been spilled in one or more of the other predecessors. The compiler of course restores the variable at the end of those predecessors, but at present we fail to recognize this. Consequently we fail to provide location information (i.e. we don't generate DBG_VALUE instructions) for the variable in BB because of seemingly conflicting locations.

This patch attempts to rectify this by recognizing restore instructions and folding them into the existing concept of 'transfers', similar to how copies are handled (see https://reviews.llvm.org/D44016).

In essence the patch consists of the following changes:

  1. Enhancing the VarLoc structure to keep track of spill locations.
  2. Introducing recognition of restore instructions analogous to recognition of spill instructions.
  3. Refactoring 'insertTransferDebugPair()' to distinguish between copies, spills and restores. This causes new DBG_VALUE instructions to be generated in response to restores.

I haven't collected any data yet on improvements to location coverage, but will do so shortly.

Diff Detail

Repository
rL LLVM

Event Timeline

wolfgangp created this revision.Jan 25 2019, 5:16 PM

Thanks, looks generally reasonable!

lib/CodeGen/LiveDebugValues.cpp
430 ↗(On Diff #183653)

as far as I can this lambda is called three times with the same arguments at the end of each if block. Why can't this code just remain at the end of the function?
Ah.. because VarLoc doesn't yet have an empty constructor?

442 ↗(On Diff #183653)

I think with three cases, this qualifies for a switch statement :-)

591 ↗(On Diff #183653)

This comment makes it sound as if this should be a method of MachineInstr used by both instead?

632 ↗(On Diff #183653)

This is almost the same function as above; how about merging the two for the sake of code size?

wolfgangp updated this revision to Diff 184006.Jan 28 2019, 6:42 PM
wolfgangp marked 5 inline comments as done.
  • Added methods to MachineInstr to recognize Spills and restores and have AsmPrinter and LIvedebugValues use them
  • Unified tranferSpill and transferRestore functions.
  • Addressed other minor review comments.
lib/CodeGen/LiveDebugValues.cpp
430 ↗(On Diff #183653)

Right. Restructuring VarLoc a bit could help with this, but the lambda seemed to be slightly less awkward option. I could attempt this if you're interested, but perhaps a different patch would be better?

591 ↗(On Diff #183653)

I gave MachineInstr a few more functions to identify spill and restore instructions. Could be done in a different patch as well.

aprantl added inline comments.Jan 28 2019, 8:47 PM
include/llvm/CodeGen/MachineInstr.h
1403 ↗(On Diff #184006)

Might be better to call this getSpillInstructionSize if it isn't returning a bool. Naming multi-purpose functions i always awkward :-)

aprantl added inline comments.Jan 29 2019, 9:40 AM
include/llvm/CodeGen/MachineInstr.h
1403 ↗(On Diff #184006)

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.

wolfgangp marked an inline comment as done.Jan 29 2019, 10:51 AM
wolfgangp added inline comments.
include/llvm/CodeGen/MachineInstr.h
1403 ↗(On Diff #184006)

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).

Changed the function names from isSpillInstruction/isRestoreInstruction to getSpillSize/getRestoreSize.

aprantl added inline comments.Jan 29 2019, 1:45 PM
include/llvm/CodeGen/MachineInstr.h
1403 ↗(On Diff #184006)

Looks good!

lib/CodeGen/LiveDebugValues.cpp
543 ↗(On Diff #184145)

I'd add something like: // This is not a spill instruction.

633 ↗(On Diff #184145)

else if

639 ↗(On Diff #184145)

else continue;
then we don't need Match any more.

wolfgangp updated this revision to Diff 184189.Jan 29 2019, 3:14 PM
  • Addressed review comments on code structure and clarifying comments.
aprantl accepted this revision.Jan 29 2019, 3:28 PM
This revision is now accepted and ready to land.Jan 29 2019, 3:28 PM
This revision was automatically updated to reflect the committed changes.