[PATCH] D39262: [CodeGen] Peel off the dominant case in switch statement in lowering

[Rong Xu via llvm-commits]

On Wed, Oct 25, 2017 at 3:31 AM, Hans Wennborg via Phabricator <
reviews at reviews.llvm.org> wrote:

> hans added a comment.
>
> Thanks! I think doing it here rather than in SimplifyCFG is much better.
>
> I think this can be made both more powerful and simpler, though.
>
> First, I think we want to peel a little later, at least after adjacent
> cases have been merged into clusters. For example if we have `case 0: case
> 1: case 2: foo;` and the three together dominate the switch, we probably
> want to peel that whole cluster instead of only peeling one case. We could
> even do this after clustering for jump tables, but maybe we don't want to
> do that. I.e. if one case dominates heavily, maybe it's worth peeling the
> case separately rather than peeling the jump table out of the switch. On
> the other hand, if it dominates completely, that jump table should be well
> predicted...
>
I was thought doing it after rangeifying clusters and peeling the merged
cluster out. but this means one extra checks for non-single case clusters.
Cluster merging does not care about the probability. There is chances we
use the extra checks for the cold cases.
My original idea was to peel out one single dominant hot case from switch.


> In any case, to do the peeling I think we should re-use the current
> lowering mechanism. This should be done at some point before creating the
> WorkList. Loop through Clusters to find one that dominates. If we find it,
> call lowerWorkItem with a single work item which represents that cluster to
> lower it, update the blocks etc. Then remove the peeled cluster from
> Clusters and proceed as usual.
>
Reusing the code in lowerWorkItem() would be good. The peeling part in
current patch is a stripped down version of lowerWorkItem(). I was worrying
that I need to update the all the probabilities of the remaining clusters.
Let me look it again to see if I can reuse the code.

>
> What do you think?
>
>
>
> ================
> Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:139
> +    "peel-dominant-case-in-lowering", cl::Hidden, cl::init(true),
> +    cl::desc("Peel the dominant case in switch statement in lowering"));
> +static cl::opt<unsigned> DominantCasePercentThreshold(
> ----------------
> Could we have just one flag instead of two separate ones? I.e. one flag
> that enables peeling and sets the threshold (and setting it to 0 would turn
> it off or something)?
>
> Will do.


>
> ================
> Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:141
> +static cl::opt<unsigned> DominantCasePercentThreshold(
> +    "dominant-case-percent-threshold", cl::Hidden, cl::init(66),
> +    cl::desc("Set the case probability threshold (<=100) for peeling out
> from "
> ----------------
> How was the value 66 chosen?
>
This is just a number I chose for now. I used some internal benchmarks to
make sure some key instances will be peeled.
I will do some more tests on this.


>
>
> https://reviews.llvm.org/D39262
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171025/d14933d3/attachment.html>