[PATCH] D70030: [MachineBlockPlacement] Update UnscheduledPredecessors when tail duplicate delete a block.

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2969
+  //     /   \
+  //  Succ0 Succ1 [BBChain]
+  if (Removed)
----------------
huihuiz wrote:
> Carrot wrote:
> > With current layout algorithm it is uncommon to build BBChain before placing PredBB, the most possible situation is BB is a loop header, and PredBB is not in the same loop. Could you write a test case with this hint?
> > 
> That's actually not right.
> 
> Consider, BBChain is the inner loop, PredBB is in the outer loop.
> BBChain for inner loop is build before outer loop (PredChain)
I can't figure out what's the difference between the two comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2970
+  //  Succ0 Succ1 [BBChain]
+  if (Removed)
+    for (MachineBasicBlock *Pred : DuplicatedPreds) {
----------------
huihuiz wrote:
> Carrot wrote:
> > Think about it more and I found it is more complex than the code snippet.
> > 
> > 1. Even BB is not completely removed, the UnscheduledPredecessors should also be decremented by the number of actual duplications. The code should be similar to the following loop.
> > 
> > 2. In your sample code, there was a UnscheduledPredecessors for the edge PredBB -> BB, even BB is duplicated into PredBB, there is still an edge (PredBB + BB) -> Succ1, so there is another  UnscheduledPredecessors, it doesn't impact the total number of UnscheduledPredecessors for BBChain. Only when BB doesn't fall through/jump to any other block in BBChain, then UnscheduledPredecessors can be decremented for the duplication. Could you double  check if your failure is in this case?
> Please refer to my test case.
> 
> The updated CFG PredBB -> Succ1 cat not make sure UnscheduledPredecessors is released through markBlockSuccessors.
> As PredBB is in the outer loop's LoopBlockSet, and Succ1 is in the inner loop's LoopBlockSet.
When work on outer loop, the BBs in inner loop are also included in LoopBlockSet. When BB is duplicated into PredBB, the edge (PredBB + BB) -> Succ1 must be added to SuccChain.UnscheduledPredecessors. Fortunately the loop at line 2982 already handled this case. So we don't need to worry about it.

But you still need to decrement BBChain.UnscheduledPredecessors for actual duplication, even if BB is not completely duplicated and removed.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2932
               RemoveList.end());
         }
 
----------------
Another related problem.
After RemBB is removed from WorkList, the following BB in the same chain should be added to WorkList.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70030