[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:158
     AU.addPreservedID(MachineLoopInfoID);
     AU.addPreservedID(MachineDominatorsID);
-    AU.setPreservesCFG();
----------------
Does it preserve MDT and MLI now?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:617
       MI->eraseFromParent();
+      if (MBB.size() == 1 && MBB.begin()->getOpcode() == AMDGPU::S_BRANCH) {
+        MachineBasicBlock *Succ = MBB.begin()->getOperand(0).getMBB();
----------------
It will work differently with and without debug info. Imagine that you have a DBG_VALUE in that block, its size will not be 1 anymore. You also need a mir test axactly the same as you are using but with the DBG_VALUE in that block.

Besides in most cases there is no S_BRANCH at the end, just a fall through to successor.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:618
+      if (MBB.size() == 1 && MBB.begin()->getOpcode() == AMDGPU::S_BRANCH) {
+        MachineBasicBlock *Succ = MBB.begin()->getOperand(0).getMBB();
+        SmallVector<MachineBasicBlock *, 2> Preds;
----------------
Isn't it the same as MBB.succ_begin()?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:620
+        SmallVector<MachineBasicBlock *, 2> Preds;
+        for (auto &Pred : MBB.predecessors()) {
+          Preds.push_back(Pred);
----------------
You can simply initialize Preds vector with MBB.predecessors(), loop is not needed.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:638
+            MachineInstr *Br =
+                BuildMI(P, DebugLoc(), TII->get(AMDGPU::S_BRANCH)).addMBB(Succ);
+            if (LIS)
----------------
Need a test for this case.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:644
+        MBB.removeSuccessor(Succ);
+        MBB.eraseFromParent();
+        if (LIS) {
----------------
What happens to MBB instructions? Will they be erased or become dangling?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:646
+        if (LIS) {
+          LIS->RemoveMachineInstrFromMaps(*MBB.begin());
+        }
----------------
MBB is already deleted at this point, it will assert or crash. It also needs to remove all instructions, not just first as there can be DBG_VALUEs.


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

https://reviews.llvm.org/D86634