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

[Huihui Zhang via Phabricator via llvm-commits]

huihuiz marked 2 inline comments as done.
huihuiz added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2970
+  //  Succ0 Succ1 [BBChain]
+  if (Removed)
+    for (MachineBasicBlock *Pred : DuplicatedPreds) {
----------------
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


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2932
               RemoveList.end());
         }
 
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70030