Page MenuHomePhabricator

[AMDGPU] Fix trivial PHI into SI_END_CF.
AbandonedPublic

Authored by hliao on Jul 18 2019, 1:31 PM.

Details

Reviewers
arsenm
rampitec
Summary
  • SI_END_CF, a MBB prologue instruction, should not take any source from PHI. But, LCSSA pass may insert one.
  • Teach PHIElim to handle such case by providing target-specific hook to get the insert point for PHI lowering (replace PHI with COPY). So that, the target could canonicalize any MBB prologue instructions.

Event Timeline

hliao created this revision.Jul 18 2019, 1:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 1:31 PM
arsenm added inline comments.Jul 18 2019, 1:42 PM
llvm/lib/CodeGen/PHIElimination.cpp
203–204

Wait, we already have isBasicBlockPrologue, and SkipPHIsAndLabels already checks it. Is it not returning true for SI_END_CF for some reason?

arsenm added inline comments.Jul 18 2019, 1:44 PM
llvm/lib/CodeGen/PHIElimination.cpp
203–204

I think to fixup the prolog uses a phi case, no new hook is necessary and can be done in terms of the existing one

hliao marked an inline comment as done.Jul 18 2019, 1:49 PM
hliao added inline comments.
llvm/lib/CodeGen/PHIElimination.cpp
203–204

that hook sounds to me should not modify anything.

How about the following simpler logic:

  • if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
  • otherwise, insert the COPY after the basic-block prologue

How about the following simpler logic:

  • if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
  • otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

It sounds to me that additional hook is a clear change. Or, we need to change the existing hook from const MachineInstr & to MachineInstr & to allow changing instruction. Any idea?

How about the following simpler logic:

  • if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
  • otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

How about the following simpler logic:

  • if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
  • otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

Unless we find a way to mark it as a prologue. An easiest (but not clean) way is to mark copy as impdef exec.

hliao added a comment.Mon, Jul 22, 6:01 AM

when we insert that copy, if we still follow the current logic of phi-elim, that COPY is already inserted after its use (that CF_END).

How about the following simpler logic:

  • if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
  • otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

Unless we find a way to mark it as a prologue. An easiest (but not clean) way is to mark copy as impdef exec.

hliao abandoned this revision.Tue, Jul 30, 3:56 PM