[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Philip Reames]

Functionality wise, this seems reasonable.  There are a few excess white 
space changes which should be removed before the patch is submitted and 
some of the new code is oddly formatted.   Once you remove the excess 
diffs, consider running clang-format.

This is not a LGTM - I don't have commit rights and don't know this code 
well enough even if I did.

Philip


On 06/10/2014 05:26 AM, Marcello Maggioni 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140610/91f03fdc/attachment.html>