[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:642
+  // increases fallthrough.
+  if (SuccSuccs.size() == 0)
+    return true;
----------------
davidxl wrote:
> I assume this is loop back edge source block. You need a test case to cover it.
test case for this?


================
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:
> > 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)
> Did you still want me to fix something here?
just add a comment above Qin decl stating that Qin is the largest frequency of Succ's incoming edges which have not been placed.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:671
+  BranchProbability PProb = MBPI->getEdgeProbability(BB, Succ);
+  BlockFrequency P = BBFreq * PProb;
+  // At this point, we don't know which block would be chosen instead of Succ.
----------------
Add a short cut here with comments:

// If P is not larger, the best successor selection loop will eventually select C, not Succ (as it is not profitable to do so).
if (P <= Qout)
   return false;


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:969
+        // This check is redundant except for look ahead. This function is
+        // called for lookahead by isProfitableToTailDup.
+        (Pred == BB))
----------------
--> ... for lookhead by isProfitableToTailDup when BB has not yet been placed.


https://reviews.llvm.org/D28583