<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 25, 2017 at 3:31 AM, Hans Wennborg via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hans added a comment.<br>
<br>
Thanks! I think doing it here rather than in SimplifyCFG is much better.<br>
<br>
I think this can be made both more powerful and simpler, though.<br>
<br>
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...<br></blockquote><div>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.</div><div>My original idea was to peel out one single dominant hot case from switch. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div>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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What do you think?<br>
<br>
<br>
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/<wbr>SelectionDAGBuilder.cpp:139<br>
+    "peel-dominant-case-in-<wbr>lowering", cl::Hidden, cl::init(true),<br>
+    cl::desc("Peel the dominant case in switch statement in lowering"));<br>
+static cl::opt<unsigned> DominantCasePercentThreshold(<br>
----------------<br>
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)?<br>
<br></blockquote><div>Will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/<wbr>SelectionDAGBuilder.cpp:141<br>
+static cl::opt<unsigned> DominantCasePercentThreshold(<br>
+    "dominant-case-percent-<wbr>threshold", cl::Hidden, cl::init(66),<br>
+    cl::desc("Set the case probability threshold (<=100) for peeling out from "<br>
----------------<br>
How was the value 66 chosen?<br></blockquote><div>This is just a number I chose for now. I used some internal benchmarks to make sure some key instances will be peeled.</div><div>I will do some more tests on this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D39262" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D39262</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>