[PATCH] D131181: [AMDGPU] Unify unreachable intrinsics

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

Thanks for working on the change! Please check the inline comment to see whether it make sense to you. I want to make it clear this is a short-term solution to workaround the limitation of structurizer, I have been working on re-writing most of the stuff in StructurizeCFG, especially the re-wire process, hope I can solve this issue in the new version. I will revert this change if the case is correctly handled.



================
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())) {
----------------
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.


================
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
----------------
Maybe something like: This used to bypass the structurization process because structurizer is unable to handle multiple-exits CFG, this should be correctly structurized.


================
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
----------------
no need to add extra check lines. you can just update the test.


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

https://reviews.llvm.org/D131181