[PATCH] D131181: [AMDGPU] Unify unreachable intrinsics

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl marked 3 inline comments as done.
yaxunl added a comment.

In D131181#3706014 <https://reviews.llvm.org/D131181#3706014>, @ruiling wrote:

>> I doubt that would help.
>
> You can have a try to remove isUniformlyReached() check for UnreachableBlock (i.e. remove the line https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp#L214) to see what would happen.

Removing isUniformlyReached() fixes the issue. Thanks. However, I got a few regressions:

  LLVM :: CodeGen/AMDGPU/GlobalISel/bool-legalization.ll
  LLVM :: CodeGen/AMDGPU/si-annotate-control-flow-condition-common-blocks.ll
  LLVM :: CodeGen/AMDGPU/skip-if-dead.ll
  LLVM :: CodeGen/AMDGPU/switch-default-block-unreachable.ll

I am checking whether these are real issues or need an update.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:214
     } else if (isa<UnreachableInst>(BB->getTerminator())) {
-      if (!isUniformlyReached(DA, *BB))
-        UnreachableBlocks.push_back(BB);
+      UnreachableBlocks.push_back(BB);
     } else if (BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator())) {
----------------
ruiling wrote:
> Please add some TODO comment to make sure this will not be missed: Actually we don't need to unify UnreachableBlocks that are uniformly reachable, we are unifying them for now to workaround the limitation of structurizer, which could not handle multiple function exits.
will do


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:214
     } else if (isa<UnreachableInst>(BB->getTerminator())) {
-      if (!isUniformlyReached(DA, *BB))
-        UnreachableBlocks.push_back(BB);
+      UnreachableBlocks.push_back(BB);
     } else if (BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator())) {
----------------
yaxunl wrote:
> ruiling wrote:
> > Please add some TODO comment to make sure this will not be missed: Actually we don't need to unify UnreachableBlocks that are uniformly reachable, we are unifying them for now to workaround the limitation of structurizer, which could not handle multiple function exits.
> will do
will do


================
Comment at: llvm/test/CodeGen/AMDGPU/si-unify-exit-multiple-unreachables.ll:9
+define amdgpu_kernel void @kernel(i32 %a, i32 addrspace(1)* %x, i32 noundef %n) {
+; Make sure unreachable blocks are unifified and branch to the unified return block
+; UNIFY-LABEL: define amdgpu_kernel void @kernel
----------------
ruiling wrote:
> Maybe something like: This used to bypass the structurization process because structurizer is unable to handle multiple-exits CFG, this should be correctly structurized.
will do


================
Comment at: llvm/test/CodeGen/AMDGPU/switch-default-block-unreachable.ll:1
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 -verify-machineinstrs -stop-after=amdgpu-unify-divergent-exit-nodes -o - %s | FileCheck -check-prefix=UNIFY %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 -verify-machineinstrs -stop-after=structurizecfg -o - %s | FileCheck -check-prefix=STRZ %s
----------------
ruiling wrote:
> no need to add extra check lines. you can just update the test.
will remove


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

https://reviews.llvm.org/D131181