Switch containing holes via table lookup, version 5

[Hans Wennborg]

Hi Jasper,

On Tue, Mar 11, 2014 at 5:12 PM, Jasper Neumann <jn at sirrida.de> wrote:
>> The most important part is missing, though: tests.
>
> I had changed one test and now have added 2 more.
> These few tests might be not enough, though.

I think they look good.

> Hans, if you think it is OK now, please check it in.

I think the patch looks good. I have a few nits below, but I've
addressed those and checked in the patch as r203694 :-)

Another idea that might be worth benchmarking is if we could combine
the "is index <= table size" and the bitmask check by anding them
together and saving a branch. It may or may not be faster, but worth
investigating.

Thanks,
Hans

> @@ -3735,11 +3736,22 @@
>    uint64_t TableSize = RangeSpread.getLimitedValue() + 1;
>    bool TableHasHoles = (NumResults < TableSize);
>
> -  // If the table has holes, we need a constant result for the default case.
> +  // if the table has holes, we need a constant result for the default case

Comments should generally start with a capital letter.

> +  bool NeedMask = (TableHasHoles && !HasDefaultResults);
> +  if (NeedMask) {
> +    // As an extra penalty for the validity test we require more cases.
> +    if (SI->getNumCases() < 4)  // TODO: Find best threshold value (benchmark).

FIXME is more common LLVM style. Please follow up on the best
threshold here (even if it turns out to be 4, so we can remove the
comment).

> +      return false;  // The tests are judged too costly.
> +    if (!(DL && DL->fitsInLegalInteger(TableSize)))
> +      return false;  // The needed mask is too big.

The two comments above seem redundant.

> +    // Build bitmask; fill in a 1 bit for every case.
> +    APInt MaskInt(TableSize, 0);
> +    APInt One(TableSize, 1);
> +    APInt Zero(TableSize, 0);

It seems Zero isn't used.

>  ; CHECK-LABEL: @nodefaultwithholes(
> -; CHECK-NOT: @switch.table
> -; CHECK: switch i32
> +; CHECK: switch.hole_check:
> +; CHECK-NEXT: %switch.maskindex = trunc i32 %switch.tableidx to i6
> +; CHECK-NEXT: %switch.shifted = lshr i6 -17, %switch.maskindex
> +; CHECK-NEXT: %switch.lobit = trunc i6 %switch.shifted to i1
> +; The mask is binary 101111.

I've expanded the test a little to make sure it branches to the right block.

> +; This lookup table will have holes, so we need to test for the holes.
> +; This is essentially the same as above but with a different mask.
> +; We build lookup tables with holes for switches with four or more cases.
> +define i32 @nodefaultwithholes2(i32 %c) {

It's not very interesting to test the same thing again. I've removed this test.

> +define i32 @threecasesholes(i32 %c) {
> +entry:
> +  switch i32 %c, label %sw.default [
> +    i32 0, label %return
> +    i32 1, label %sw.bb1
> +    i32 3, label %sw.bb2
> +  ]
> +sw.bb1: br label %return
> +sw.bb2: br label %return
> +sw.default: br label %return
> +return:
> +  %x = phi i32 [ 3, %sw.default ], [ 5, %sw.bb2 ], [ 7, %sw.bb1 ], [ 9, %entry ]
> +  ret i32 %x
> +; CHECK-LABEL: @threecasesholes(
> +; CHECK-NOT: switch i32
> +; CHECK: @switch.table

This test looks wrong. %sw.default should not yield a constant value,
and the CHECKs should check that we're not building a lookup table.