[PATCH] D86634: [AMDGPU] SILowerControlFlow::optimizeEndCF should remove empty basic block

[Alexander via Phabricator via llvm-commits]

alex-t added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:697
+        MachineInstr *Br =
+            BuildMI(P, DebugLoc(), TII->get(AMDGPU::S_BRANCH)).addMBB(Succ);
+        if (LIS)
----------------
rampitec wrote:
> You do not need this branch if target is a fall thru block.
> You also do not seem a testcase when it is not and this branch is needed.
Removing block we channge MBBs order.
Verifier complains if the new successor is not a branch target and is not "next node" (MBB.getNextNode()).
It consider legal fall thru to the next node only.

The branch is only necessary in case of fall thru to make verifier happy



================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:158
     AU.addPreservedID(MachineLoopInfoID);
     AU.addPreservedID(MachineDominatorsID);
-    AU.setPreservesCFG();
----------------
rampitec wrote:
> alex-t wrote:
> > rampitec wrote:
> > > Does it preserve MDT and MLI now?
> > yes. your initial assumption was a chain of single successors. So, if we remove block, we remove it from that chain.
> I assume when you remove a block you also need to call at least MLI::removeBlock() and MDT::eraseNode() to call them preserved.
Oops. Yes. This is correct.  
So, given that neither MDT no LI is not used in SILowerControlFlow I'd perfer to remove setPreserved. Otherwise I'd call getAnalysisIfAvailable just to get a pointer and update them.  I'm not sure if it worth doing. 


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

https://reviews.llvm.org/D86634