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

[Samuel Pitoiset via Phabricator via llvm-commits]

hakzsam added a comment.

Thanks for your feedback.

I will make a patch for the second issue.



================
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();
----------------
nhaehnle wrote:
> 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.
Probably not, but using a else-if seems like safer.


================
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:
----------------
nhaehnle wrote:
> It would be good to check that there is no spurious branch /after/ the conditional branch either. This probably works:
> 
>   CHECK-NEXT: bb.3:
> 
Yes, I will fix that.


https://reviews.llvm.org/D28351