[PATCH] D28583: CodeGen: Allow small copyable blocks to "break" the CFG.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:413
+  /// fallthroughs.
+  bool probabilityJustifiesTailDuplicate(
+      MachineBasicBlock *BB, MachineBasicBlock *Succ);
----------------
Suggest new name : isProfitableToTailDup


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:612
+  //               |/
+  //               Dom
+  // In the second case, Placing Succ while duplicating it into C prevents the
----------------
Dom -> PDom


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:619
+  for (MachineBasicBlock *SuccSucc : Succ->successors()) {
+    auto Prob = MBPI->getEdgeProbability(Succ, SuccSucc);
+    if (Prob > BestSuccSucc)
----------------
Why not just check if there exists a SuccSucc that post dominates Succ directly?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:638
+  //  Cost in the first case is: P + V
+  //  Cost in the second case is: Q + QV + PU + PV
+  if (Dom == nullptr || !Succ->isSuccessor(Dom)) {
----------------
PU + PV == P

Also Assuming U > V, the layout order (with tail dup) on should be  

BB --> Succ --> D 

so the overall cost is:

Q + P V + Q ( which is smaller than Q + QV + PU + PV)


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:639
+  //  Cost in the second case is: Q + QV + PU + PV
+  if (Dom == nullptr || !Succ->isSuccessor(Dom)) {
+    BranchProbability P = (MBPI->getEdgeProbability(BB, Succ));
----------------
We can not assume BB and Succ are in a triangular shape subcfg here -- given where this method is called. Besides, if Succ is not tail-duped, the layout decision may even reject Succ as the layout successor, so the cost is no longer P + V, but 2*Q + V instead (with U > V). 

In other words, isProfitable check can not be done inside 'hasBetterLayoutPredecessor', but hoisted to the caller of it when 'hasBetterLayoutPrecessor' returns, at which point we will know the layout decision if taildup does not kick in.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:654
+  BranchProbability Q = MBPI->getEdgeProbability(BB, Succ).getCompl();
+  // If there is a post-dominating successor, here is the calculation:
+  // BB         BB
----------------
In this case, what is needed to to invoke 'hasBetterLayoutPredecessor' on PDom block. Dependinng the result, we will know that without tailDup, the layout order is Succ-> PDom or Succ->D->PDom. This will make the cost computation more precise.


https://reviews.llvm.org/D28583