[PATCH] D63489: [InstSimplify] LCSSA PHIs should not be simplified away

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D63489#1548945 <https://reviews.llvm.org/D63489#1548945>, @arsenm wrote:

> In D63489#1548938 <https://reviews.llvm.org/D63489#1548938>, @alex-t wrote:
>
> > In D63489#1548851 <https://reviews.llvm.org/D63489#1548851>, @arsenm wrote:
> >
> > > In D63489#1548844 <https://reviews.llvm.org/D63489#1548844>, @lebedev.ri wrote:
> > >
> > > > In D63489#1548837 <https://reviews.llvm.org/D63489#1548837>, @alex-t wrote:
> > > >
> > > > > In D63489#1548470 <https://reviews.llvm.org/D63489#1548470>, @lebedev.ri wrote:
> > > > >
> > > > > > In D63489#1548451 <https://reviews.llvm.org/D63489#1548451>, @alex-t wrote:
> > > > > >
> > > > > > > In D63489#1548288 <https://reviews.llvm.org/D63489#1548288>, @lebedev.ri wrote:
> > > > > > >
> > > > > > > > > This causes generation of incorrect code in AMDGPU backend.
> > > > > > > >
> > > > > > > > This sounds like some other check is missing elsewhere?
> > > > > > > >  What happens if you feed it such an ir as-if after this transform, but manually written?
> > > > > > > >  ("that will result in broken asm/crashes" is hopefully not the answer)
> > > > > > > >
> > > > > > > > That being said, why is `LCSSAPass` not sufficient?
> > > > > > > >  It's already supposed to undo transforms like this.
> > > > > > >
> > > > > > >
> > > > > > > It will result in syntactically correct asm and no crashes. In runtime we'll get incorrect result though :)
> > > > > > >  Adding LCSSA pass again later on is difficult in the sense of the pass dependencies.
> > > > > > >  So, it's better to fix the explicit bug in SimplifyPHI....
> > > > > >
> > > > > >
> > > > > > Aha, so it's not `-instsimplify` pass itself, but how it's used during transition into backend.
> > > > > >
> > > > > > 1. You certainly don't want to make this blacklist unconditional, it should still run when the `-instsimplify` pass itself is run. (+instsimplify test)
> > > > > > 2. How does this affect other targets (backends)? Does this need some TLI hook?
> > > > >
> > > > >
> > > > > In fact I still insist that this is a bug in -instsimplify pass.
> > > > >  The algorithm is written in such a way that it always expect more then one input in PHI node.
> > > > >  That means that the person who's written it had no intention to really remove the LCSSA PHIs but was just unaware of their existence!
> > > > >  It is not correct to remove LCSSA in similar manner as PHI nodes with equal inputs are removed.
> > > > >  So, any other backend should not suffer from this change. It is not about the convenience for AMDGPU backend but about correctness.
> > > > >  Could you provide an example where LCSSA PHIs are added and then intentionally removed?
> > > >
> > > >
> > > > Can you please specify why this transform is invalid from LLVM IR point of view? https://godbolt.org/z/D8gKNc
> > > >  In `endloop` BB, which has a single predecessor BB - `loop`, `%counter.lcssa` value can only be `%counter` value.
> > >
> > >
> > > Nothing is invalid from the IR point of view. This is all a kludge to get divergence information into SelectionDAG. There needs to be an IR instruction at the use point for the DAG to query the divergence analysis
> >
> >
> > That means I have to change all the function declarations on the way to be able to pass TTI to check if it is the "divergent" target (
>
>
> I don't think InstSimplify should depend on target information


Can't you just schedule an `LCSSAPass` somewhere late in pipeline, before SelectionDAG and after early-cse?


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

https://reviews.llvm.org/D63489