[PATCH] D68092: [AMDGPU] Invert the handling of skip insertion.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:113-114
+  TII->analyzeBranch(SrcMBB, TrueMBB, FalseMBB, Cond);
+  if (!FalseMBB)
+    FalseMBB = SrcMBB.getNextNode();
+
----------------
I think this reinterpreting analyzeBranch's outputs the way is potentially confusing.

I think you don't actually need to check analyzeBranch directly here; I think MachineBasicBlock::getFallThrough does exactly this anyway (and handles the case where there's an unconditional branch as well)


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:116-117
+
+  if (MDT->dominates(TrueMBB, &SrcMBB) ||
+      mustRetainExeczBranch(*FalseMBB, *TrueMBB))
+    return false;
----------------
cdevadas wrote:
> nhaehnle wrote:
> > What's the logic here behind using domination as a criterion?
> There could be a situation in which execnz (inserted during SI_LOOP lowering) can be inverted to execz by an optimization (for instance, BranchFolding). This execz should always be retained. This special check is added to handle it.
> Unfortunately, I couldn't write/find a test-case to reproduce it.
I'm not sure dominance is sufficient for irreducible loops, which you won't run into in practice (as in, they probably hit another control flow bug long before this) but we should handle it correctly


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68092