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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:116-117
+
+  if (MDT->dominates(TrueMBB, &SrcMBB) ||
+      mustRetainExeczBranch(*FalseMBB, *TrueMBB))
+    return false;
----------------
arsenm wrote:
> 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
I have seen irreducible loops go all the way through compilation (because they triggered a bug somewhere, I believe in waitcount insertion), so yeah, that needs to be handled correctly.

I still think a reasonable way to do this is just to scan forward like mustRetainExeczBranch already does, see if we encounter the execz target block during that scan, and only remove the execz branch in that case.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68092