[PATCH] D64353: [AMDGPU] Run '' after isel to simplify PHIs.

[Michael Liao via Phabricator via llvm-commits]

hliao added a comment.

Please review https://reviews.llvm.org/D64946 following your suggestion.

In D64353#1590117 <https://reviews.llvm.org/D64353#1590117>, @arsenm wrote:

> I think  PHIElimination::LowerPHINode needs to be taught to look for prolog instructions
>
> In D64353#1590052 <https://reviews.llvm.org/D64353#1590052>, @hliao wrote:
>
> > In D64353#1590037 <https://reviews.llvm.org/D64353#1590037>, @arsenm wrote:
> >
> > > No matter what, running UnreachableMachineBlockElim is not a fix for this.
> > >
> > > In D64353#1574085 <https://reviews.llvm.org/D64353#1574085>, @hliao wrote:
> > >
> > > > In D64353#1574073 <https://reviews.llvm.org/D64353#1574073>, @arsenm wrote:
> > > >
> > > > > In D64353#1574005 <https://reviews.llvm.org/D64353#1574005>, @hliao wrote:
> > > > >
> > > > > > In D64353#1574004 <https://reviews.llvm.org/D64353#1574004>, @arsenm wrote:
> > > > > >
> > > > > > > In D64353#1573974 <https://reviews.llvm.org/D64353#1573974>, @hliao wrote:
> > > > > > >
> > > > > > > > In D64353#1573922 <https://reviews.llvm.org/D64353#1573922>, @arsenm wrote:
> > > > > > > >
> > > > > > > > > I think you should probably fix SILowerControlFlow
> > > > > > > >
> > > > > > > >
> > > > > > > > the misplacement of def/use is not the fault of SILowerControlFlow but PHIEliminate. However, even PHIEliminate should not be blamed as cf.end is part of MBB prologue. cf.end should not take any input from PHI.
> > > > > > > >
> > > > > > > > In O2 <https://reviews.llvm.org/owners/package/2/> compilation, there won't be such issue as, in optimized RA, livevariable is used to eliminate PHI better. LiveVariables depends on unreachable-mbb-elimin, which remove the PHI to cf.end.
> > > > > > >
> > > > > > >
> > > > > > > This isn't a property the pass should rely on. SILowerControlFlow should defend against unexpected iteration order and unreachable blocks
> > > > > >
> > > > > >
> > > > > > any use before def is invalid MIR, right?
> > > > >
> > > > >
> > > > > So the MIR is valid before phi elimination, and then becomes invalid after. Removing unreachable blocks earlier to avoid this happening is a workaround. It seems to me like we need to fix PHI elimination or avoid relying on the prolog instructions
> > > >
> > > >
> > > > my understanding is that MBB prologue instructions are designed NOT to take PHI as input. Like these pseudo instructions for structurized CFG, by design, they don't take any PHI as input. As an alternative approach (not workaround), we could ensure these instructions not taking PHI as input before PHIElim instead of teaching PHIElim to under them, more or less very target-specific stuff.
> > >
> > >
> > > The verifier should probably check this. We definitely should not hack around this by deleting unreachable blocks that happens to run into this
> >
> >
> > But, the verifier so far doesn't change that. For your concerns, I'd like to take the following approach:
> >
> > - Add target checker to assert those instructions with direct PHI input.
> > - Add a special pass after isel to canonicalize these pseudo CFG instructions by removing PHI inputs.
> >
> >   Does that sound to be acceptable?
>





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64353