[PATCH] D133840: AMDGPU: Add a pass to rewrite certain undef in PHI

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

In D133840#3791543 <https://reviews.llvm.org/D133840#3791543>, @ruiling wrote:

> In D133840#3791463 <https://reviews.llvm.org/D133840#3791463>, @sameerds wrote:
>
>> In D133840#3791418 <https://reviews.llvm.org/D133840#3791418>, @ruiling wrote:
>>
>>> Move the pass after SIAnnotateControlFlow.
>>> Simplify the backedge undef logic since joining divergent threads at loop header has been handled by SIAnnotateControlFlow.
>>
>> Is there an advantage in making this assumption that it always runs after SIAnnotateControlFlow? It makes the pass less useful for anything other than the current AMDGPU. If there is no harm in keeping the backedge logic, then the pass will remain useful for any future use where such loops do occur.
>
> Please see my inline comment, we cannot work correctly for such kind of loops which may appear before SIAnnotateControlFlow for now. We should move the block-split logic inside SIAnnotateControlFlow into structurizer itself later, then the pass should be able to work with any pass after StructurizeCFG.

I understand that the block-split produces new opportunities for uniform phis, so we want to run this pass after "SIAnnotateControlFlow". But that does not automatically mean that we should assume that this is the *only* place it will ever run. By "work correctly", do you mean that the original handling of back-edges will produce incorrect IR? If not, that treatment of back-edges need not be removed.

Is there also an assumption that this pass works only with a structurized CFG? If both StructurizeCFG and SIAnnotateControlFlow are *necessary* for this pass to produce correct IR, then this needs to be document. If they are not, then this pass should be designed to work even without them, in order to preserve future opportunity. If loop backedges need extra work, at least we should document it in the code comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133840