[PATCH] D82641: [AMDGPU] Unify early PS termination blocks

Mon Jun 29 05:54:20 PDT 2020

nhaehnle added a comment.

In D82641#2118340 <https://reviews.llvm.org/D82641#2118340>, @cdevadas wrote:

> We are trying to have the kill intrinsic handling early during the instruction selection.

Whether this is a good idea depends on what is meant by it :) Kill intrinsics interact with WQM for how pixel shaders are defined, and we really need to move the WQM pass later because it adds instructions that interfere with scheduling in a bad way. Setting up the CFG to be the right shape earlier makes sense to me.

Mostly looks good to me, some minor inline comments, but most importantly: please add a test case in skip-if-dead.ll with a function that triggers the early-exit and returns a value (the only test return right now is `@test_kill_control_flow`, but it doesn't trigger this early exit).

================
Comment at: llvm/lib/Target/AMDGPU/SIInsertSkips.cpp:167

+static void generateEndPgm(MachineBasicBlock &MBB,
+                           MachineBasicBlock::iterator I, DebugLoc DL,
----------------
Should probably be renamed to `generatePsEndPgm`.

================
Comment at: llvm/lib/Target/AMDGPU/SIInsertSkips.cpp:222-223
+      createEarlyExitBlock(MBB);
+      // Reset iterator as next block may have changed
+      NextBBI = std::next(MBB.getIterator());
+    }
----------------
Why the update to NextBBI? The old code didn't do that, I believe on purpose because while it doesn't break correctness, there simply shouldn't be a need to inspect the EarlyExitBlock.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82641