[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Marcello Maggioni]

I think you are probably right. I don't have a lot of experience with
phabricator, so I avoided it until now, but I'll make sure next time to use
it, seems definitely better for this .

Marcello


2014-06-12 18:49 GMT+01:00 Eric Christopher <echristo at gmail.com>:

> Also, FWIW, it's easier to review some of this stuff via phabricator :)
>
> -eric
>
> On Thu, Jun 12, 2014 at 10:39 AM, Jim Grosbach <grosbach at apple.com> wrote:
> > 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.
> >
> >
> > +// 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/
> >
> > +    // 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.
> >
> > +  // NewLowerBound here should never be the integer value minimal.
> >
> >
> > I don’t quite follow this comment.
> >
> > +  // 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.
> >
> >
> > 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/20140613/3f83a1c8/attachment.html>