[PATCH] Improving LowerSwitch behavior for contiguous ranges

Fri Jun 13 19:56:05 PDT 2014

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/20140614/096fd2c1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: optimized_lower_switch_v7.patch
Type: application/octet-stream
Size: 17751 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140614/096fd2c1/attachment.obj>