<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 18, 2017 at 12:14 PM, 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 inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>SimplifyCFG.cpp:5680<br>
<br>
+  if (PeelDominantCase(SI, Builder))<br>
+    return SimplifyCFG(BB, TTI, BonusInstThreshold, AC) | true;<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> hans wrote:<br>
> > I don't think we should do this if SwitchToLookupTable (the call below) can turn the swich into a lookup instead.<br>
> ><br>
> > In fact, I don't think this should be done at the IR level at all, since it's more of a switch lowering issue. SelectionDAGBuilder::<wbr>visitSwitch() would be a better place.<br>
> ><br>
> > That code already takes case weights into account, and when lowering to a binary search tree, it will balance it based on weight, favoring cases that are hot. Do you find that that's not sufficient? (I'm willing to believe that's the case, but I'd like to see it argued.)<br>
> Peeling the  dominating case avoids a memory read (table lookup). Besides it trades a indirect branch for a a highly biased direct branch (which is usually highly predictable by the branch predictor).<br>
><br>
> Like looping which happens at higher level,  switch peeling can also enable more possible surrounding optimizations (eg like jump threading).  Peeling it out also probably also makes it easier to for better code layout.<br>
><br>
> On the other hand, this should not be done when size optimization is on.<br>
><br>
><br>
><br>
</span>SwitchToLookupTable creates lookup tables for switches that are used to select from a set of constant values (this is different from jump tables, sorry the names here are confusing), so there is no indirect branch. I think peeling off the common case is probably not a good idea for those kinds of switches.<br>
<br>
I can see that peeling early might enable other high-level optimizations. But that's true in some sense for switch lowering in general. I'd suggest to at least look into how peeling in the regular switch lowering code at SelectionDAGBuilder::<wbr>visitSwitch() would look like. It seems to me like the natural place to do it, and I suspect it's easier to do there.<br>
<br></blockquote><div>Thanks for the suggestion. Doing this late does avoid some interactions with the folding in simplifyCFG. I'll implement this in lowering phrase and compare the performance.  </div><div><br></div><div>-rong</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D37940" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37940</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>