[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Eric Christopher]

On Tue, Jun 10, 2014 at 11:37 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Eric Christopher" <echristo at gmail.com>
>> To: "Marcello Maggioni" <hayarms at gmail.com>
>> Cc: llvm-commits at cs.uiuc.edu
>> Sent: Tuesday, June 10, 2014 1:28:44 PM
>> Subject: Re: [PATCH] Improving LowerSwitch behavior for contiguous ranges
>>
>> On Tue, Jun 10, 2014 at 11:24 AM, Marcello Maggioni
>> <hayarms at gmail.com> wrote:
>> > Hello,
>> >
>> > thank you for your feedback guys!
>> >
>> > Answers to everybody:
>> >
>> > - Pete: Thanks for that, seems like clang-format-diff doesn't
>> > understand
>> > DEBUG() macros ... :P
>>
>> Might want to file a bug and let Daniel Jasper know :)
>>
>> > - Philip: I removed as much whitespace changes as I could find
>> > - Eric: I changed the comment in head to switchConvert to explain
>> > the new
>> > Upper and LowerBound arguments (is that what you meant?)
>> >
>>
>> Yep. Though I have to admit that it'd be nice if we had a range
>> propagation pass that would clean this up rather than special casing.
>> :\
>
> Doesn't LVI do range propagation?
>

"Some" is what I know. I'm curious why we don't get this case.

-eric

>  -Hal
>
>>
>> -eric
>>
>> > Cheers,
>> > Marcello
>> >
>> >
>> > 2014-06-10 18:14 GMT+01:00 Eric Christopher <echristo at gmail.com>:
>> >
>> >> While you're in there updating the documentation for switchConvert
>> >> would be good as well.
>> >>
>> >> -eric
>> >>
>> >> On Tue, Jun 10, 2014 at 5:26 AM, Marcello Maggioni
>> >> <hayarms at gmail.com>
>> >> wrote:
>> >> > Hello,
>> >> >
>> >> > when LowerSwitch transforms a switch instruction into a tree of
>> >> > ifs it
>> >> > is
>> >> > actually performing a binary search into the various case
>> >> > ranges, to see
>> >> > if
>> >> > the current value falls into one cases range of values.
>> >> >
>> >> > So, if we have a program with something like this:
>> >> >
>> >> > switch (a) {
>> >> > case 0:
>> >> >   do0();
>> >> >   break;
>> >> > case 1:
>> >> >   do1();
>> >> >   break;
>> >> > case 2:
>> >> >   do2();
>> >> >   break;
>> >> > default:
>> >> >   break;
>> >> > }
>> >> >
>> >> > the code produced is something like this:
>> >> >
>> >> >   if (a < 1) {
>> >> >     if (a == 0) {
>> >> >       do0();
>> >> >     }
>> >> >   } else {
>> >> >     if (a < 2) {
>> >> >       if (a == 1) {
>> >> >         do1();
>> >> >       }
>> >> >     } else {
>> >> >       if (a == 2) {
>> >> >         do2();
>> >> >       }
>> >> >     }
>> >> >   }
>> >> >
>> >> > This code is inefficient because the check (a == 1) to execute
>> >> > do1() is
>> >> > not
>> >> > needed.
>> >> >
>> >> > The reason is that because we already checked that (a >= 1)
>> >> > initially by
>> >> > checking that also  (a < 2) we basically already inferred that
>> >> > (a == 1)
>> >> > without the need of an extra basic block spawned to check if
>> >> > actually (a
>> >> > ==
>> >> > 1).
>> >> >
>> >> > The proposed patch addresses this problem by keeping track of
>> >> > already
>> >> > checked bounds in the LowerSwitch algorithm, so that when the
>> >> > time
>> >> > arrives
>> >> > to produce a Leaf Block that checks the equality with the case
>> >> > value /
>> >> > range
>> >> > the algorithm can decide if that block is really needed
>> >> > depending on the
>> >> > already checked bounds .
>> >> >
>> >> > For example, the above with "a = 1" would work like this:
>> >> >
>> >> > the bounds start as LB: NONE , UB: NONE
>> >> > as (a < 1) is emitted the bounds for the else path become LB: 1
>> >> > UB:
>> >> > NONE.
>> >> > This happens because by failing the test (a < 1) we know that
>> >> > the value
>> >> > "a"
>> >> > cannot be smaller than 1 if we enter the else branch.
>> >> > After the emitting the check (a < 2) the bounds in the if branch
>> >> > become
>> >> > LB:
>> >> > 1 UB: 1. This is because by checking that "a" is smaller than 2
>> >> > then the
>> >> > upper bound becomes 2 - 1 = 1.
>> >> >
>> >> > When is time to emit the leaf block for "case 1:" we notice that
>> >> > 1 is
>> >> > squeezed exactly in between the LB and UB, which means that if
>> >> > we
>> >> > arrived to
>> >> > that block there is no need to emit a block that checks if (a ==
>> >> > 1).
>> >> >
>> >> >
>> >> > Please let me know if I did something wrong here and if the
>> >> > reasoning is
>> >> > correct :)
>> >> >
>> >> > The proposed patch also sets as XFAIL the test:
>> >> > "test/Transforms/LowerSwitch/feature.ll".
>> >> > This test checks that the emitted lowered CFG is exactly the one
>> >> > expected by
>> >> > the previous algorithm without this optimization. Of course this
>> >> > test
>> >> > starts
>> >> > failing when this optimization is added. I added another test
>> >> > that is
>> >> > the
>> >> > same, but is updated with the new expected CFG.
>> >> >
>> >> > Don't know what is the best thing to do with the original
>> >> > "feature.ll"
>> >> > test
>> >> > in the case this patch gets accepted. Should we remove it or
>> >> > keep it as
>> >> > a
>> >> > reference?
>> >> >
>> >> > Cheers,
>> >> > Marcello
>> >> >
>> >> > _______________________________________________
>> >> > 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
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory