<div dir="ltr">I think you are probably right. I don't have a lot of experience with phabricator, so I avoided it until now, but I'll make sure next time to use it, seems definitely better for this .<div><br></div>
<div>Marcello</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-12 18:49 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">Also, FWIW, it's easier to review some of this stuff via phabricator :)<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Thu, Jun 12, 2014 at 10:39 AM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:<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>
> +// 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>
> + // 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>
> + // NewLowerBound here should never be the integer value minimal.<br>
><br>
><br>
> I don’t quite follow this comment.<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>
> 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 up<br>
> 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>
> <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>
> 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>