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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:712
+    }
+    MBB.eraseFromParent();
+    if (MDT)
----------------
Will this also erase all instructions?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:713
+    MBB.eraseFromParent();
+    if (MDT)
+      MDT->eraseNode(&MBB);
----------------
I would do it before actually erasing MBB itself.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:715
+      MDT->eraseNode(&MBB);
+    if (MLI)
+      MLI->removeBlock(&MBB);
----------------
Looks like both MLI and MDT are already destroyed at this point anyway. Do you see any of the analysis available? If not I would just remove it from preserved.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:697
+        MachineInstr *Br =
+            BuildMI(P, DebugLoc(), TII->get(AMDGPU::S_BRANCH)).addMBB(Succ);
+        if (LIS)
----------------
alex-t wrote:
> 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
> 
This defeats optimization purpose to reduce control flow. This branch will survive till the end, right?


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

https://reviews.llvm.org/D86634