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

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

I'll be glad to add some more comments to explain, but I think the calculations are correct. I've commented individually.



================
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));
----------------
davidxl wrote:
> 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.
I think I have the calculation right for when Succ would not be the layout successor, but you're right to point out that we should also do the calculation even when Succ is the chosen successor.





================
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;
+
----------------
davidxl wrote:
> There is a reason SmallSetVector is used here -- to make sure the iteration order is deterministic.
BlockFilterSet is never iterated. I checked.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:653
+        PDom = SuccSucc;
+        break;
+      }
----------------
davidxl wrote:
> Why break here?
Because if PDom is not null, that's all that we look at for the probability calculation.


================
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:
----------------
davidxl wrote:
> 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)
When we place Succ, we remove 2 fallthrough edges BB->C and C'->Succ.

Freq(C'->Succ) may be larger than Freq(BB->C).
I am using Qin to represent Freq(C'->Succ) and Qout for Freq(BB->C). I could just use different letters if that were more clear.

Qout is Freq(BB->C). I don't think Qin should be as well.


================
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;
----------------
davidxl wrote:
> PDom is always a successor of Succ according to the way it is computed.
Thanks.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:697
+    BlockFrequency BaseCost = P + V;
+    BlockFrequency DupCost = Qout + QinV + P * AdjustedSuccSumProb;
+    return (BaseCost > DupCost);
----------------
davidxl wrote:
> 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
This function is called in a loop looking for the highest probability successor. If Q > P, this function will be ignored and we will lay out Q anyway, so we can ignore the second case.

As to the first case:
Until the 2nd patch lands, the duplication will prevent the BB->Succ->D layout.

Instead you will get BB->Succ ; C'->D
So the cost is as calculated.
D28522 will include an update to this calculation along with an update to the behavior.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:840
+///    considered
+/// \p Lookahead: if non-null, a set of blocks to ignore.
 bool MachineBlockPlacement::hasBetterLayoutPredecessor(
----------------
davidxl wrote:
> Add more description about what blocks to ignore.
Well, that's really up to the caller. Do you want me to list why you might want to ignore a block?


https://reviews.llvm.org/D28583