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

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

Let me know what you think. There is a small cleanup that could go in as a separate patch: I switched to a SmallDenseSet because we don't need the orderedness of the SmallVectorSet.



================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:638
+  //  Cost in the first case is: P + V
+  //  Cost in the second case is: Q + QV + PU + PV
+  if (Dom == nullptr || !Succ->isSuccessor(Dom)) {
----------------
davidxl wrote:
> PU + PV == P
> 
> Also Assuming U > V, the layout order (with tail dup) on should be  
> 
> BB --> Succ --> D 
> 
> so the overall cost is:
> 
> Q + P V + Q ( which is smaller than Q + QV + PU + PV)
I thought that too. But without the lattice patch, after duplication, we won't put D after Succ because it now has an unplaced predecessor. The lattice patch fixes the behavior and the calculation.


================
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: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.
We now only call this function to check if we should use Succ despite it having been rejected. So we know that Succ is not the layout successor.


https://reviews.llvm.org/D28583