[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;
+
----------------
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:
> > 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?
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 ||
+        (LookAhead && LookAhead->count(Pred)))
       continue;
----------------
I think it is equivalent to check Pred == BB. In normal calling context, this is covered by BlockToChain[Pred] == &Chain, but for lookahead case, it is needed to filter BB which is not laid out yet.


https://reviews.llvm.org/D28583