[PATCH] D28351: AMDGPU: Remove spurious out branches after a kill

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

nhaehnle added a comment.

Thanks! We generally use CHECK-LABEL only for the start of a function, not for labels inside a function. The idea is that the label helps FileCheck get less confused when there are many sub-tests in a single test file.

There's still the issue of what happens when the target of the unconditional branch isn't the next block. That's a pre-existing bug, so you don't necessarily have to fix it in this commit, but it would be good if you could tackle this before we forget about it.

More comments inline.



================
Comment at: lib/Target/AMDGPU/SIInsertSkips.cpp:282-288
         if (MBB.isLayoutSuccessor(MI.getOperand(0).getMBB()))
           MI.eraseFromParent();
+        // Remove the given unconditional branch when a skip block has been
+        // inserted after the current one and let skip the two instructions
+        // performing the kill if the exec mask is non-zero.
+        if (HaveSkipBlock)
+          MI.eraseFromParent();
----------------
There's still a potential double-eraseFromParent, isn't there? Or I guess not because the skip block is always in the way, but still, I think this should be unified or at least an else-if.


================
Comment at: test/CodeGen/AMDGPU/insert-skips-kill-uncond.mir:17-19
+# CHECK-NEXT: S_CBRANCH_EXECNZ %bb.2, implicit %exec
+
+# CHECK-LABEL: bb.3:
----------------
It would be good to check that there is no spurious branch /after/ the conditional branch either. This probably works:

  CHECK-NEXT: bb.3:



https://reviews.llvm.org/D28351