[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Eric Christopher]

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.
:\

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