[PATCH] Improving LowerSwitch behavior for contiguous ranges
Eric Christopher
echristo at gmail.com
Sat Jun 14 22:55:16 PDT 2014
(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