[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Eric Christopher]

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
>> >
>
>