[PATCH] D102830: [AMDGPU] Avoid null export insertion when unifying exit blocks

[Carl Ritson via Phabricator via llvm-commits]

critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:154
 
-BasicBlock *AMDGPUUnifyDivergentExitNodes::unifyReturnBlockSet(
-    Function &F, DomTreeUpdater &DTU, ArrayRef<BasicBlock *> ReturningBlocks,
-    bool InsertExport, StringRef Name) {
-  // Otherwise, we need to insert a new basic block into the function, add a PHI
-  // nodes (if the function returns values), and convert all of the return
-  // instructions into unconditional branches.
-  BasicBlock *NewRetBlock = BasicBlock::Create(F.getContext(), Name, &F);
-  IRBuilder<> B(NewRetBlock);
+// Search a block and its predecessors to find a "done" export
+static IntrinsicInst *findExportDone(BasicBlock *BB,
----------------
foad wrote:
> critson wrote:
> > foad wrote:
> > > Isn't it possible that different predecessors have different "done" exports? What is this function supposed to do in that case?
> > This does assumes the IR is well-formed to a certain degree.
> > I have added a comment to try to document this.
> > 
> > Consider the following IR:
> > ```if (condition) {
> >  export done 1
> > } else {
> >  export done 2
> > }
> > return```
> > 
> > This will eventually be compiled to:
> > ```set exec mask for if-branch
> > export done 1
> > set exec mask for else-branch
> > export done 2
> > restore exec mask
> > return```
> > 
> > Multiple export done instructions will be executed which is invalid.
> > The expected cases for this are:
> > 
> > 1. Divergent exits with their own exports, e.g.
> > ```if (condition) {
> >   export done 1
> >   return
> > } else {
> >   export done 2
> >   return
> > }```
> > 
> > 2. Or divergent exits without exports or a uniformly reached export, e.g.
> > ```export done
> > if (condition) {
> >   return
> > } else {
> >   return
> > }```
> OK. Thanks for explaining. I find it hard to get my head round the possible cases that we do or don't have to handle. Do you cope with:
> ```
> if (uniform condition)
>   export done 1
> else
>   export done 2
> if (divergent condition)
>   return 1
> else
>   return 2
> ```
> ?
Good question, I think the answer is no -- although it is in principle valid IR that we do not generate.
I realize I was thinking primarily of divergent conditions, rather than uniform ones.
Although the comment I added is still accurate, <= 1 reachable export done.

In principle I can rework this to handle the case you raised, but I am not sure it is worth it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102830