[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:2969
+  //     /   \
+  //  Succ0 Succ1 [BBChain]
+  if (Removed)
----------------
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)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70030