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

[Jay Foad via Phabricator via llvm-commits]

foad 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,
----------------
critson wrote:
> 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?
I have no idea what cases must be handled or are "worth" handling, so I think I'll leave that part of the review to someone who understands that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102830