[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:268
   /// \brief A typedef for a block filter set.
-  typedef SmallSetVector<MachineBasicBlock *, 16> BlockFilterSet;
+  typedef SmallDenseSet<MachineBasicBlock *, 16> BlockFilterSet;
+
----------------
There is a reason SmallSetVector is used here -- to make sure the iteration order is deterministic.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:642
+  // increases fallthrough.
+  if (SuccSuccs.size() == 0)
+    return true;
----------------
I assume this is loop back edge source block. You need a test case to cover it.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:653
+        PDom = SuccSucc;
+        break;
+      }
----------------
Why break here?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:658
+  // from BB.
+  auto BestSuccPred = BlockFrequency(0);
+  for (MachineBasicBlock *SuccPred : Succ->predecessors()) {
----------------
nit: -->SuccBestPred


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:666
+    if (Freq > BestSuccPred)
+      BestSuccPred = Freq;
+  }
----------------
Computing BestSuccPred here is unnecessary. See below for more comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:675
+  BlockFrequency Qout = BBFreq * (AdjustedSumProb - PProb);
+  BlockFrequency Qin = BestSuccPred;
+  // If it doesn't have a post-dominating successor, here is the calculation:
----------------
Qin is not necessarily BestSuccPred. 

Profitability check is called only after hasBetterLayoutPredecessor is returned and it returns true.  There are two scenarios it returns true
1) Qin or Qout is larger than P, or
2) P is larger than Qout, but not the branch is not biased enough such that the layout algorithm still decides to keep the top-order.

Either way,  the baseline layout to compare (with taildup) is that BB->Succ is the branch taken edge, and BB->C is the fall through edge.  Qin should just be Prob(BB->C)


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:691
+  //  Cost in the second case is: Qout + Qin * V + P * U + P * V
+  if (PDom == nullptr || !Succ->isSuccessor(PDom)) {
+    BranchProbability UProb = BestSuccSucc;
----------------
PDom is always a successor of Succ according to the way it is computed.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:697
+    BlockFrequency BaseCost = P + V;
+    BlockFrequency DupCost = Qout + QinV + P * AdjustedSuccSumProb;
+    return (BaseCost > DupCost);
----------------
The base cost is as wrote,  the DupCost however depends on whether P > Q or not.

If P > Q, the fallthrough path is BB->Succ->D
so the cost  (normalized with freq(bb) ==1) is
   2*Q+ P*V

If P < Q, the fall through path is    BB->C'->D
the cost is
  2*P + Q*V


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:840
+///    considered
+/// \p Lookahead: if non-null, a set of blocks to ignore.
 bool MachineBlockPlacement::hasBetterLayoutPredecessor(
----------------
Add more description about what blocks to ignore.


https://reviews.llvm.org/D28583