[PATCH] Improving LowerSwitch behavior for contiguous ranges
Jim Grosbach
grosbach at apple.com
Mon Jun 16 10:04:49 PDT 2014
Committed as r211038. Thanks, Marcello.
I used the description from the original email in the thread for a commit message.
-Jim
> On Jun 14, 2014, at 10:55 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> (Oh, Jim had said he was going to commit so I'll let him)
>
> -eric
>
> On Sat, Jun 14, 2014 at 10:54 PM, Eric Christopher <echristo at gmail.com> wrote:
>> Awesome. Thanks.
>>
>> LGTM at this point.
>>
>> -eric
>>
>> On Fri, Jun 13, 2014 at 7:56 PM, Marcello Maggioni <hayarms at gmail.com> wrote:
>>> Hi Eric,
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> If somebody else has comments about this let me know.
>>>
>>> Marcello
>>>
>>>
>>> 2014-06-13 18:28 GMT+01:00 Eric Christopher <echristo at gmail.com>:
>>>
>>>> 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
>>>>>
>>>
>>>
More information about the llvm-commits
mailing list