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

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

In https://reviews.llvm.org/D28583#653869, @davidxl wrote:

> Please mark addressed comments as done.  Also let me know if it is ready for another round of review (I saw some issues not addressed such as the deterministic iteration of block filter set).


Marked.

I think it's ready, and I put back the deterministic set.



================
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:
> 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?


https://reviews.llvm.org/D28583