[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Marcello Maggioni]

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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140613/118eb4da/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: optimized_lower_switch_v6.patch
Type: application/octet-stream
Size: 17605 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140613/118eb4da/attachment.obj>