[PATCH] D22892: Codegen: MachineBlockPlacement Improve probability layout.

[Xinliang David Li via llvm-commits]

 A better way to illustrate forked diamond

  //       H
  //      / \
  //     /   \
  //   BB    Pred
  //   / \  /|
  //   |  X  |
  //   | / \ |
  //   S1   S2
  //    \   /
  //      M


On Wed, Jul 27, 2016 at 6:47 PM, David Li <davidxl at google.com> wrote:

> davidxl added a comment.
>
> This is a good change. I actually wanted to get rid of the forward check
> which is always true for case 2 and redundant for case 1. Now it is also
> bad for case 3.
>
> Please add a test case also.
>
>
> ================
> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:639
> @@ +638,3 @@
> +  //     /   \
> +  //   BB    Pred
> +  //   / \  /   \
> ----------------
> Nit: can you make the art work like the following to not split S2 into two
> 'blocks':
>
> //
>   //      Head (Or Entry, Top)
>   //      / \
>   //     /   \
>   //   BB    Pred
>   //   / \   /  |
>   //   |  S1  |
>   //    \      /
>   //      S2
>   //
>
>
> ================
> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:643
> @@ +642,3 @@
> +  //
> +  // The current block is BB and edge BB->S1 is now being evaluated.
> +  // As above S->BB was already selected because
> ----------------
> Nit: can you make the art work like the following to not split S2 into two
> 'blocks':
>
> //
>   //      Head (Or Entry, Top)
>   //      / \
>   //     /   \
>   //   BB    Pred
>   //   / \   /  |
>   //   |  S1  |
>   //    \      /
>   //      S2
>   //
>
>
> ================
> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:660
> @@ +659,3 @@
> +  // Non-topo-order cost:
> +  // In the worst case, S2 will not get layed out after Pred.
> +  // non-topo-cost = 2 * freq(S->Pred) + freq(BB->S2).
> ----------------
> layed out --> laid out
>
> ================
> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:661
> @@ +660,3 @@
> +  // In the worst case, S2 will not get layed out after Pred.
> +  // non-topo-cost = 2 * freq(S->Pred) + freq(BB->S2).
> +  // To be conservative, we can assume that min(freq(Pred->S1),
> freq(Pred->S2))
> ----------------
> Another way to explain in terms of savings instead of cost: the savings is
> the total freq of the fall through edges. In topo case, the savings is
>
> freq(S->BB) + max(freq(Pred->S1), freq(Pred->S2).   (1)
>
> For non-top case, the saving is:
>
> freq(S->BB) + freq(BB->S1) + freq(Pred->S2)          (2)
>
> When freq(Pred->S2) > freq(Pred->S1), (2) is strictly larger than (1). In
> the opposite case, the check below will also lead to (2) > (1)
>
> ================
> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:679
> @@ +678,3 @@
> +    // Do backward checking.
> +    // For case 2 above, we need a backward checking to filter out edges
> that
> +    // are not 'strongly' biased. With profile data available, the check
> is
> ----------------
> For case 1 and 2
>
> ================
> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:683
> @@ -661,1 +682,3 @@
> +    // than two successors.
> +    // For case 3 above, this test is essential, even with profiling data.
>      // BB  Pred
> ----------------
> This comment does not fit here. With profile data, such check won't be
> skipped, but just does not need to be as biased.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D22892
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160727/b58859a3/attachment.html>