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

David Li via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 20 16:19:35 PST 2017

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

for (MachineBasicBlock *LoopBB : LoopBlockSet)
fillWorkLists(LoopBB, UpdatedPreds, &LoopBlockSet);

================
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:
----------------
iteratee wrote:
> 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.
differentiate Qin and Qout is fine, but in the code Qin = BestSuccPred which could be Freq(BB->Succ).  What I meant is you should directly compute Qin as its definition Freq(C'->Succ)

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:697
+    BlockFrequency BaseCost = P + V;
+    BlockFrequency DupCost = Qout + QinV + P * AdjustedSuccSumProb;
+    return (BaseCost > DupCost);
----------------
iteratee wrote:
> 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.
You are right about Q > P case that that scenario will be dropped. It is very subtle, so please add some comment to clarify.

Ok -- for the first case, also add a comment

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:840
+///    considered
+/// \p Lookahead: if non-null, a set of blocks to ignore.
bool MachineBlockPlacement::hasBetterLayoutPredecessor(
----------------
iteratee wrote:
> davidxl wrote:
> Well, that's really up to the caller. Do you want me to list why you might want to ignore a block?
something like : e.g, when called under xxx, we want to ignore yyy. See caller zzz for details.   However, see my comment in the function,  this parameter seems unnecessary.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:974
+        BlockToChain[Pred] == &Chain ||