Page MenuHomePhabricator

[LiveVariables] Mark PhysReg implicit-def MachineOperands of INLINEASM_BR as LiveOut
Needs ReviewPublic

Authored by nickdesaulniers on Apr 28 2020, 4:56 PM.



D78586 implements good sanity checks in MachineVerifier that for each
LiveIn register to a MachineBasicBlock, that at least one predecessor
MachineBasicBlock defines the register as LiveOut.

The PhysReg MachineOperands for INLINEASM_BR MachineInstrs were being
implicit-def'd but marked dead (i.e. no uses in local MachineBasicBlock)
by live-vars.

ScheduleDAGSDNodes::EmitSchedule() splits the MachineBasicBlock
containing the INLINEASM_BR, post that MachineInstr, and marks the
LiveIns. But there's no marking of LiveOuts. If a use is visible within
the same MachineBasicBlock, live-vars will calculate the LiveOuts

Since the uses aren't visible in the same MachineBasicBlock as the
INLINEASM_BR MachineInstr, teach LiveVariables::runOnBlock() to special
case INLINEASM_BR's PhysReg defs, so that they're not marked dead.

Implementing TCOPY (D75098) should also make the uses visible to
live-vars as well, at which point, this change to LiveVariables should
be reverted (but not the tests).

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 4:56 PM
efriedma added inline comments.Apr 28 2020, 6:34 PM

I'm concerned about the "allowing it to do a single local analysis to resolve physical register lifetimes in each basic block" part. Is this change enough to actually make the liveness reliable? In particular, I'm concerned what might happens if the fallthrough successor of an INLINEASM_BR has other predecessors. (Or is that impossible?)


How is EarlyClobber related to this?

void added inline comments.May 2 2020, 4:56 AM

Drive-by comment:

INLINEASM_BR's fallthrough successor can have multiple predecessors, at least in theory (I don't believe there's a validation check preventing it). E.g.:

a(bool b) {
  if (b)
    asm goto("" : : : : indirect1);
    asm goto("" : : : : indirect2);
  // Both "asm goto"s fallthrough to here.

In the IR, I believe clang generates a fallthrough block for each "asm goto" even though they both end up falling through to the same place. But because of general transformations, those can be elided of course, especially when converted to machine instructions.

My original implementation created a separate "copy" MBB that has only one predecessor---the block with the INLINEASM_BR instruction. It acts as a block that would be created during critical edge splitting.