Page MenuHomePhabricator

[DebugInstrRef][1/3] Track PHI values through register allocation
ClosedPublic

Authored by jmorse on Aug 28 2020, 1:21 PM.

Details

Summary

(Context: the patch series' at D83046 and D85741). This series tracks PHI instructions through register allocation -- which in the original RFC, I wrote:

"My understanding is that the register allocation phase of LLVM starts there [phielim] and ends after virtregrewriter, and it'd be legitimate to say "we do special things for these passes"."

This patch introduces the "special things": in PHI Elimination we record the position of any PHI instruction that is eliminated, its register and debug instruction number. The register is updated through register allocation; and afterwards, we emit a 'DBG_PHI' instruction identifying the physreg / stack slot where the PHI occurs.

An example of the DBG_PHI instruction working would be:

bb1:
   DBG_PHI $rax, 1
   [... more insts ... ]
bb2:
   DBG_INSTR_REF 1, 0

The DBG_PHI identifies the position at which the PHI occurs ($rax here) for debug instruction 1, which can then be picked up by instruction-referencing LiveDebugValues, and tracked until a DBG_INSTR_REF instruction uses that debug instruction number.

This may look a bit like a DBG_VALUE, but it isn't: by using two instructions, we've separated two really important parts of variable value tracking:

  • Where the machine value comes from (specified by DBG_PHI),
  • Where the machine value becomes the variables value.

For PHI instructions, we absolutely need a program point to identify the machine value: that's the nature of PHIs. Thus, to track PHI values, we need to put a new instruction to identify where the PHI occurs.

In this particular patch, I've added boilerplate for the DBG_PHI instruction, some data structures in MachineFunction to track PHIs during regalloc, and code in LiveDebugVariables to update locations when virtual registers are split plus emit DBG_PHIs afterwards. In the other two patches:

  • Register coalescing
  • Handling this all in LiveDebugValues

Diff Detail

Event Timeline

jmorse created this revision.Aug 28 2020, 1:21 PM
jmorse requested review of this revision.Aug 28 2020, 1:21 PM
aprantl added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
467

nit /// comes before the field, ///< after.

llvm/lib/CodeGen/LiveDebugVariables.cpp
554

do we need the sorted-ness of the map for determinism, or could we use a DenseMap?

llvm/lib/CodeGen/PHIElimination.cpp
324

insert({ID, p});

jmorse updated this revision to Diff 290943.Sep 10 2020, 4:55 AM

Address comments; add a test for creating DBG_PHIs for PHIs that happen in spill slots.

(I also suggested a test was needed for sub-register inputs; however on further examination, you can't PHI unequal register classes, so subregisters don't enter the picture at that stage).

jmorse marked 3 inline comments as done.Sep 10 2020, 4:55 AM
dstenb added a subscriber: dstenb.Sep 10 2020, 9:01 AM

Just some drive-by nits while familiarizing myself with this patch series.

llvm/include/llvm/CodeGen/MachineFunction.h
463

maintenance.

llvm/lib/CodeGen/LiveDebugVariables.cpp
551

///

llvm/lib/CodeGen/PrologEpilogInserter.cpp
1273

Nit: Capital letter and period.

jmorse updated this revision to Diff 290997.Sep 10 2020, 9:09 AM
jmorse marked 2 inline comments as done.

Revise some dodgy comments, cheers!

jmorse edited the summary of this revision. (Show Details)Oct 14 2020, 9:29 AM

ping; (also remove from summary the reference to additional tests needed, as these are now in the patch).

'Tis the season to ping!

Note that I'm very comfortable with Stephens patches having a higher priority than these; they're less of a conceptual difference and appear to be yielding a good improvement :)

LGTM, some nits inline. Thanks.

llvm/include/llvm/CodeGen/MachineFunction.h
476

Do we need the std::map sortness? Can we use llvm::DenseMap instead?

llvm/lib/CodeGen/LiveDebugVariables.cpp
1433

should be capital letter

1841–1843

I guess we do not need the else here.

llvm/lib/CodeGen/PHIElimination.cpp
323–324

these should be capital letters, I guess

llvm/lib/CodeGen/PrologEpilogInserter.cpp
1273

the comment should go above the line

llvm/test/DebugInfo/MIR/InstrRef/phi-regallocd-to-stack.mir
22–28

unused, could be deleted

llvm/test/DebugInfo/MIR/InstrRef/phi-through-regalloc.mir
51–52

unused, could be deleted

jmorse updated this revision to Diff 346720.May 20 2021, 7:24 AM

Ping and update for @djtodoro 's comments -- some minor editing and deleting of un-necessary parts of tests.

Now that I'm confident that the overall approach is sound (see D102158), I'd like to put this patch series back into peoples minds again.

Promoting from inline:

Do we need the std::map sortness? Can we use llvm::DenseMap instead?

No, and I've removed it in this revision. I've been pursuing a "don't prematurely optimise" approach during development, but it makes sense to optimise when actually committing things.

NB: I'll probably drop this in sometimes soon, on the back of @djtodoro 's LGTM earlier

djtodoro accepted this revision.Tue, May 25, 11:06 AM

Still LGTM.

This revision is now accepted and ready to land.Tue, May 25, 11:06 AM
This revision was landed with ongoing or failed builds.Wed, May 26, 12:24 PM
This revision was automatically updated to reflect the committed changes.