[PATCH] Improving LowerSwitch behavior for contiguous ranges
Marcello Maggioni
hayarms at gmail.com
Mon Jun 16 10:56:26 PDT 2014
Thanks Jim :)
Marcello
2014-06-16 18:04 GMT+01:00 Jim Grosbach <grosbach at apple.com>:
> 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
> >>>>>
> >>>
> >>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140616/fe092938/attachment.html>
More information about the llvm-commits
mailing list