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

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas marked 2 inline comments as done.
cdevadas added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRemoveShortExecBranches.cpp:113-114
+  TII->analyzeBranch(SrcMBB, TrueMBB, FalseMBB, Cond);
+  if (!FalseMBB)
+    FalseMBB = SrcMBB.getNextNode();
+
----------------
arsenm wrote:
> 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)
The check was necessary when it is a direct fallthrough; the analyzeBranch returns without assigning the FalseMBB. All I eventually require is FalseMBB field.

MachineBasicBlock::getFallThrough returns the fallthrough branch and doesn't serve the real purpose (getting the FlaseMBB) esp. when the false path is taken via. an unconditional branch.
With the following sequence, for instance, getFallThrough() returns %bb.1. But what we need is %bb.3  
  bb.0:
          successors: %bb.3, %bb.1
          -------------
          S_CBRANCH_EXECZ %bb.1
          S_BRANCH %bb.3
    bb.1:
          ;  predecessors: %bb.0, %bb.3
          successors: %bb.2, %bb.4

I believe, extracting the FalseMBB from the successor_list would be a better idea. 
The SrcMBB will always have exactly two successors.
 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68092