[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Jim Grosbach]

Thanks. This LGTM. If no-one else has any comments, I'll get this committed for you later today.

Jim

> On Jun 13, 2014, at 5:11 AM, Marcello Maggioni <hayarms at gmail.com> wrote:
> 
> Thanks Jim :)
> 
> Answers inlined
> 
> 
> 2014-06-12 18:39 GMT+01:00 Jim Grosbach <grosbach at apple.com>:
> Hi Marcello,
> 
> This is awesome. I’ve been wanting improvements like this for a long time now, but have never gotten around to doing it myself. My procrastination^Wpatience is paying off!
> 
> When running on the LLVM test suite (including externals, preferably), how often does this optimization fire?
> 
> A few detail comments below. Nothing major.
> 
> General style nitpick: make sure comments are full sentences and end in a period.
> 
> 
> 
> Thank you for the suggestion Jim!
> About the number of triggers of the optimization in the standard LLVM tests the switch lowering is not run in a lot of tests (8 tests in total I believe). 
> When it ran though the the optimization was triggered 3 times out of 8.
> 
> In externals where was used much more I saw it triggering about 15%.
> 
>> +// LowerBound and UpperBound are used to keep track of the bounds for Val       
>> +// that have being already checked by a block emitted by one of the previous    
>> +// calls to switchConvert in the call stack.   
> 
> s/have being already/have already been/
> 
> 
> Fixed :P 
>> +    // Check if the Case Range is perfectly squeezed in between
>> +    // already checked Upper and Lower bounds. If it is then we can avoid
>> +    // emitting the code that checks if the value actually falls in the range
>> +    // because the bounds already tell us so
>> +    if (LowerBound != nullptr && UpperBound != nullptr &&
>> +        Begin->Low == LowerBound && Begin->High == UpperBound) {
>> +      return Begin->BB;
>> +    }
> 
> Can Begin->Low or Begin->High ever be null? If not, the explicit nullptr checks can be removed as they’re implicit. If they can be null, that seems a bit odd for the algorithm.
> 
>  
> Oh, good spot here! Low and High actually cannot be null. Thanks. 
>> +  // NewLowerBound here should never be the integer value minimal.
> 
> I don’t quite follow this comment.
> 
> 
> Yeah, it could have been more explanatory ... :P
> What I mean here is that the NewLowerBound variable cannot have assigned the smallest value an integer value of the type the switch is evaluating can encode (like -128 for an i8 for example) and that is safe to do -1 without overflowing.
> The reason for that is that NewLowerBound is never computed from the case with the lowest value, so if there is at least one case with a lower value of the one we are computing that value from it means it cannot have assigned the lowest available value and we can subtract at least one from it.
> 
> A little bit convoluted but I hope it explained it ... 
> 
> I added a more descriptive comment and also some extra comments related to that.
>> +  // Optimize the case where Default is an unreachable block                    
>> +  if (DefaultIsUnreachable) {                                                   
>> +    CaseItr LastCase = Cases.begin() + Cases.size() - 1;                        
>> +    UpperBound = cast<ConstantInt>(LastCase->High);                             
>> +    LowerBound = cast<ConstantInt>(Cases.begin()->Low);                         
>> +  }
> 
> 
> While accurate, the comment is a bit terse. It would be helpful to explain how we’re optimizing it, not just that we are. I.e., explain why the following code results in an optimization.
> 
> 
> 
> Tried to make it a little bit more descriptive :) 
> 
> Marcello
>> On Jun 11, 2014, at 4:22 PM, Marcello Maggioni <hayarms at gmail.com> wrote:
>> 
>> I'm updating the patch to also remove the default basic block if it ends up being dead after switch lowering (no predecessors)
>> 
>> Marcello
>> 
>> 2014-06-11 20:34 GMT+01:00 Marcello Maggioni <hayarms at gmail.com>:
>> Joerg,
>> 
>> I addressed your suggestion in this patch.
>> It was quite easy to add and can be useful in general, so thanks!
>> 
>> I also added a test that tests this kind of optimization being applied.
>> 
>> Marcello
>> 
>> 
>> 2014-06-11 18:37 GMT+01:00 Joerg Sonnenberger <joerg at britannica.bec.de>:
>> 
>> On Wed, Jun 11, 2014 at 05:28:05PM +0100, Marcello Maggioni wrote:
>> > Yeah, it is suboptimal, it doesn't take into consideration the fact that
>> > the default is unreachable.
>> >
>> > I'll a look at it later to see if it is easy to also take into
>> > consideration this case and add it to the patch + test for the condition.
>> 
>> Thanks. Please don't take this as hold up for pushing the original
>> patch, it can just as well be a follow-up commit.
>> 
>> Joerg
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>> <optimized_lower_switch_v5.patch>_______________________________________________
>> 
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> <optimized_lower_switch_v6.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140613/d7d07fe3/attachment.html>