[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Eric Christopher]

Looks pretty good to me. Couple of comments:

+  ConstantInt *NewUpperBound = nullptr;

Probably don't need to initialize this since it's initialized
unconditionally on the next set of conditionals. (And you assume as
much in the DEBUG statement below :)

If the testcases came straight from some C/C++ it's occasionally nice
to have the code in the testcase IMO (others may disagree).

-eric


On Fri, 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
>>
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>