<div dir="ltr">Hi Eric,<div><br></div><div>I integrated your suggestion about the initialization and I also added the source code of the only test derived directly from source in the test itself.</div><div><br></div><div>I honestly don't know what is the policy about this, but I saw that other tests also contain the source code, so I added it.</div>
<div><br></div><div>If somebody else has comments about this let me know.</div><div><br></div><div>Marcello</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-13 18:28 GMT+01:00 Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On Fri, Jun 13, 2014 at 5:11 AM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>> 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 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), 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 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 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 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 avoid<br>
>> +    // emitting the code that checks if the value actually falls in 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 nullptr<br>
>> checks can be removed as they’re implicit. If they can be null, that 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 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 without<br>
> overflowing.<br>
> The reason for that is that NewLowerBound is never computed from the case<br>
> with the lowest value, so if there is at least one case with a lower value<br>
> of the one we are computing that value from it means it cannot have 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 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 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>> wrote:<br>
>><br>
>> I'm updating the patch to also remove the default basic block if it 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 applied.<br>
>>><br>
>>> Marcello<br>
>>><br>
>>><br>
>>> 2014-06-11 18:37 GMT+01:00 Joerg Sonnenberger <<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>
>> <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>
</div></div></blockquote></div><br></div>