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

[Alexander via Phabricator via llvm-commits]

alex-t added a comment.

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?


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

https://reviews.llvm.org/D63489