[PATCH] Improving LowerSwitch behavior for contiguous ranges

[David Blaikie]

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?

Most likely we should just change the original to test the new
behavior, and not introduce a new test. If the original test doesn't
test all cases of the new behavior and can't be reasonably made to do
so, we could introduce a new test for those new cases (while keeping
the old one covering the existing cases, but updated to verify the new
optimization you've implemented is functioning)

- David

>
> Cheers,
> Marcello
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>