<div dir="ltr">Thanks Jim :)<div><br></div><div>Marcello</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-16 18:04 GMT+01:00 Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Committed as r211038. Thanks, Marcello.<br>
<br>
I used the description from the original email in the thread for a commit message.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Jim<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> On Jun 14, 2014, at 10:55 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
><br>
> (Oh, Jim had said he was going to commit so I'll let him)<br>
><br>
> -eric<br>
><br>
> On Sat, Jun 14, 2014 at 10:54 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
>> Awesome. Thanks.<br>
>><br>
>> LGTM at this point.<br>
>><br>
>> -eric<br>
>><br>
>> On Fri, Jun 13, 2014 at 7:56 PM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>> wrote:<br>
>>> Hi Eric,<br>
>>><br>
>>> I integrated your suggestion about the initialization and I also added the<br>
>>> source code of the only test derived directly from source in the test<br>
>>> itself.<br>
>>><br>
>>> I honestly don't know what is the policy about this, but I saw that other<br>
>>> tests also contain the source code, so I added it.<br>
>>><br>
>>> If somebody else has comments about this let me know.<br>
>>><br>
>>> Marcello<br>
>>><br>
>>><br>
>>> 2014-06-13 18:28 GMT+01:00 Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>>:<br>
>>><br>
>>>> Looks pretty good to me. Couple of comments:<br>
>>>><br>
>>>> +  ConstantInt *NewUpperBound = nullptr;<br>
>>>><br>
>>>> Probably don't need to initialize this since it's initialized<br>
>>>> unconditionally on the next set of conditionals. (And you assume as<br>
>>>> much in the DEBUG statement below :)<br>
>>>><br>
>>>> If the testcases came straight from some C/C++ it's occasionally nice<br>
>>>> to have the code in the testcase IMO (others may disagree).<br>
>>>><br>
>>>> -eric<br>
>>>><br>
>>>><br>
>>>> On Fri, Jun 13, 2014 at 5:11 AM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>><br>
>>>> wrote:<br>
>>>>> Thanks Jim :)<br>
>>>>><br>
>>>>> Answers inlined<br>
>>>>><br>
>>>>><br>
>>>>> 2014-06-12 18:39 GMT+01:00 Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>>:<br>
>>>>><br>
>>>>>> Hi Marcello,<br>
>>>>>><br>
>>>>>> This is awesome. I’ve been wanting improvements like this for a long<br>
>>>>>> time<br>
>>>>>> now, but have never gotten around to doing it myself. My<br>
>>>>>> procrastination^Wpatience is paying off!<br>
>>>>>><br>
>>>>>> When running on the LLVM test suite (including externals, preferably),<br>
>>>>>> how<br>
>>>>>> often does this optimization fire?<br>
>>>>>><br>
>>>>>> A few detail comments below. Nothing major.<br>
>>>>>><br>
>>>>>> General style nitpick: make sure comments are full sentences and end in<br>
>>>>>> a<br>
>>>>>> period.<br>
>>>>>><br>
>>>>>><br>
>>>>><br>
>>>>> Thank you for the suggestion Jim!<br>
>>>>> About the number of triggers of the optimization in the standard LLVM<br>
>>>>> tests<br>
>>>>> the switch lowering is not run in a lot of tests (8 tests in total I<br>
>>>>> believe).<br>
>>>>> When it ran though the the optimization was triggered 3 times out of 8.<br>
>>>>><br>
>>>>> In externals where was used much more I saw it triggering about 15%.<br>
>>>>><br>
>>>>>> +// LowerBound and UpperBound are used to keep track of the bounds for<br>
>>>>>> Val<br>
>>>>>> +// that have being already checked by a block emitted by one of the<br>
>>>>>> previous<br>
>>>>>> +// calls to switchConvert in the call stack.<br>
>>>>>><br>
>>>>>><br>
>>>>>> s/have being already/have already been/<br>
>>>>>><br>
>>>>><br>
>>>>> Fixed :P<br>
>>>>>><br>
>>>>>> +    // Check if the Case Range is perfectly squeezed in between<br>
>>>>>> +    // already checked Upper and Lower bounds. If it is then we can<br>
>>>>>> avoid<br>
>>>>>> +    // emitting the code that checks if the value actually falls in<br>
>>>>>> the<br>
>>>>>> range<br>
>>>>>> +    // because the bounds already tell us so<br>
>>>>>> +    if (LowerBound != nullptr && UpperBound != nullptr &&<br>
>>>>>> +        Begin->Low == LowerBound && Begin->High == UpperBound) {<br>
>>>>>> +      return Begin->BB;<br>
>>>>>> +    }<br>
>>>>>><br>
>>>>>><br>
>>>>>> Can Begin->Low or Begin->High ever be null? If not, the explicit<br>
>>>>>> nullptr<br>
>>>>>> checks can be removed as they’re implicit. If they can be null, that<br>
>>>>>> seems a<br>
>>>>>> bit odd for the algorithm.<br>
>>>>>><br>
>>>>><br>
>>>>> Oh, good spot here! Low and High actually cannot be null. Thanks.<br>
>>>>>><br>
>>>>>> +  // NewLowerBound here should never be the integer value minimal.<br>
>>>>>><br>
>>>>>><br>
>>>>>> I don’t quite follow this comment.<br>
>>>>>><br>
>>>>><br>
>>>>> Yeah, it could have been more explanatory ... :P<br>
>>>>> What I mean here is that the NewLowerBound variable cannot have assigned<br>
>>>>> the<br>
>>>>> smallest value an integer value of the type the switch is evaluating can<br>
>>>>> encode (like -128 for an i8 for example) and that is safe to do -1<br>
>>>>> without<br>
>>>>> overflowing.<br>
>>>>> The reason for that is that NewLowerBound is never computed from the<br>
>>>>> case<br>
>>>>> with the lowest value, so if there is at least one case with a lower<br>
>>>>> value<br>
>>>>> of the one we are computing that value from it means it cannot have<br>
>>>>> assigned<br>
>>>>> the lowest available value and we can subtract at least one from it.<br>
>>>>><br>
>>>>> A little bit convoluted but I hope it explained it ...<br>
>>>>><br>
>>>>> I added a more descriptive comment and also some extra comments related<br>
>>>>> to<br>
>>>>> that.<br>
>>>>>><br>
>>>>>> +  // Optimize the case where Default is an unreachable block<br>
>>>>>> +  if (DefaultIsUnreachable) {<br>
>>>>>> +    CaseItr LastCase = Cases.begin() + Cases.size() - 1;<br>
>>>>>> +    UpperBound = cast<ConstantInt>(LastCase->High);<br>
>>>>>> +    LowerBound = cast<ConstantInt>(Cases.begin()->Low);<br>
>>>>>> +  }<br>
>>>>>><br>
>>>>>><br>
>>>>>> While accurate, the comment is a bit terse. It would be helpful to<br>
>>>>>> explain<br>
>>>>>> how we’re optimizing it, not just that we are. I.e., explain why the<br>
>>>>>> following code results in an optimization.<br>
>>>>>><br>
>>>>>><br>
>>>>><br>
>>>>> Tried to make it a little bit more descriptive :)<br>
>>>>><br>
>>>>> Marcello<br>
>>>>>><br>
>>>>>> On Jun 11, 2014, at 4:22 PM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>><br>
>>>>>> wrote:<br>
>>>>>><br>
>>>>>> I'm updating the patch to also remove the default basic block if it<br>
>>>>>> ends<br>
>>>>>> up being dead after switch lowering (no predecessors)<br>
>>>>>><br>
>>>>>> Marcello<br>
>>>>>><br>
>>>>>> 2014-06-11 20:34 GMT+01:00 Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>>:<br>
>>>>>>><br>
>>>>>>> Joerg,<br>
>>>>>>><br>
>>>>>>> I addressed your suggestion in this patch.<br>
>>>>>>> It was quite easy to add and can be useful in general, so thanks!<br>
>>>>>>><br>
>>>>>>> I also added a test that tests this kind of optimization being<br>
>>>>>>> applied.<br>
>>>>>>><br>
>>>>>>> Marcello<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> 2014-06-11 18:37 GMT+01:00 Joerg Sonnenberger<br>
>>>>>>> <<a href="mailto:joerg@britannica.bec.de">joerg@britannica.bec.de</a>>:<br>
>>>>>>><br>
>>>>>>>> On Wed, Jun 11, 2014 at 05:28:05PM +0100, Marcello Maggioni wrote:<br>
>>>>>>>>> Yeah, it is suboptimal, it doesn't take into consideration the fact<br>
>>>>>>>>> that<br>
>>>>>>>>> the default is unreachable.<br>
>>>>>>>>><br>
>>>>>>>>> I'll a look at it later to see if it is easy to also take into<br>
>>>>>>>>> consideration this case and add it to the patch + test for the<br>
>>>>>>>>> condition.<br>
>>>>>>>><br>
>>>>>>>> Thanks. Please don't take this as hold up for pushing the original<br>
>>>>>>>> patch, it can just as well be a follow-up commit.<br>
>>>>>>>><br>
>>>>>>>> Joerg<br>
>>>>>>>> _______________________________________________<br>
>>>>>>>> llvm-commits mailing list<br>
>>>>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>>>><br>
>>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> <optimized_lower_switch_v5.patch>_______________________________________________<br>
>>>>>><br>
>>>>>> llvm-commits mailing list<br>
>>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>>><br>
>>>>>><br>
>>>>><br>
>>>>><br>
>>>>> _______________________________________________<br>
>>>>> llvm-commits mailing list<br>
>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>><br>
>>><br>
>>><br>
<br>
</div></div></blockquote></div><br></div>