[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:2970
+  //  Succ0 Succ1 [BBChain]
+  if (Removed)
+    for (MachineBasicBlock *Pred : DuplicatedPreds) {
----------------
huihuiz wrote:
> Carrot wrote:
> > 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.
> When work on outer loop, the BBs in inner loop are NOT included in LoopBlockSet for the outer loop. 
> 
> 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.
> -> also not right.
> -> The PredBB is not in the LoopBlockSet for SuccChain, the SuccChain.UnscheduledPredecessors has been increased in respect to PredBB earlier in fillWorkList
> 
> But you still need to decrement BBChain.UnscheduledPredecessors for actual duplication, even if BB is not completely duplicated and removed.
> -> if BB is not completely removed, then should leave to markBlockSuccessors to decrease the UnscheduledPredecessors counter
Please read the code in function collectLoopBlockSet. Also it is intentionally to leave cold BBs from inner loop to be processed with outer loop to get better I$ behavior.

For the handling of UnscheduledPredecessors. It records the number of cross chain edges.
In normal situation, it is constructed in fillWorkList, and decreased when predecessor is placed.
But when tail duplication occurred, the number of cross chain edges changed, so UnscheduledPredecessors must be adjusted accordingly. There are two possibilities:
  * reduced cross chain edges, this is the root cause of your problem, but a completely duplicated and removed BB is really a special case.
  * increased cross chain edges, it is handled by loop at line 2982, please read it carefully, it is a good template for your patch.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2932
               RemoveList.end());
         }
 
----------------
huihuiz wrote:
> Carrot wrote:
> > Another related problem.
> > After RemBB is removed from WorkList, the following BB in the same chain should be added to WorkList.
> There is no need for special handling, when done building Loop Chain, all MBB within the function will be checked again to see if there's need to be added to WorkList. Refer to Line:2604
Please keep in mind this file tries to layout code optimally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70030