Switch containing holes via table lookup, version 4

[Hans Wennborg]

I think the patch is starting to look pretty reasonable.

The most important part is missing, though: tests.

On Sat, Mar 8, 2014 at 3:01 AM, Jasper Neumann <jn at sirrida.de> wrote:
> Hello Hans, hello folks,
>
> let me thank you for helping me getting this little patch to work.
>

>>> +  if (NeedMask && !DL->fitsInLegalInteger(TableSize))
>
>> DL can be NULL here.
>> This is causing the segfaults in the lit tests you mentioned.
>
> Thank you for finding this bug!
> I assume that if DL is NULL we cannot do anything and therefore have to bail
> out.

Right.

>> It feels a little odd that we kind of take LookupBB here, rename it,
>> and then create a new one :) The code above which creates branches to
>> LookupBB is actually branching to this hole_check bb.
>
>> I wonder if it would be easier to follow if we first created LookupBB,
>> then if NeedMask is true we create HoleCheckBB. When we create the
>> branches above we branch to HoleCheckBB if it's non-NULL, and LookupBB
>> otherwise.
>
> The problem is that the BB descriptions match the following block and not
> the block currently dealt with.
> I have thought a while about this problem. My solution introduces the new
> block only when needed and all the code is generated within one if.
> Your approach exchanges the renaming oddity with additional tests, i.e. the
> code gets longer.
> Which approach is better readable might depend on personal taste.

OK. I think the comments you added make this more clear.


Some more comments below. It feels like we're down to the last details now :)

Thanks,
Hans


> @@ -3745,12 +3746,25 @@
>    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...

I don't like the "..." style of this and the next comment. How about
just changing to say that if the table has holes, we need a constant
result or a bitmask that fits in a register.

>    SmallVector<std::pair<PHINode*, Constant*>, 4> DefaultResultsList;
> -  if (TableHasHoles && !GetCaseResults(SI, 0, SI->getDefaultDest(), &CommonDest,
> -                                       DefaultResultsList, DL))
> -    return false;
> +  bool HasCaseResults = false;

I thought you said you'd renamed this?

> +  if (TableHasHoles) {
> +    HasCaseResults = GetCaseResults(SI, 0, SI->getDefaultDest(), &CommonDest,
> +                                    DefaultResultsList, DL);
> +  }
> +  // ...or need to check for valid values with a sufficiently small bit set.
> +  bool NeedMask = (TableHasHoles && !HasCaseResults);
> +  if (NeedMask) {
> +    // As an extra penalty for the validity test we require more cases.
> +    if (SI->getNumCases() < 4)  // TODO: Make threshold configurable.

I don't think we should make the threshold configurable; I think we
should find the right one. 4 sounds reasonable to me.

> +      return false;  // The tests are judged too costly.
> +    if (!(DL && DL->fitsInLegalInteger(TableSize)))
> +      return false;  // The needed mask is too big.
> +  }
>
> +  // Now we either can fill all holes (if present) with the default value
> +  // or we can check for the holes with a suitable bitmask.

This comment looks a little out of place. It looks like it's attached
to the for loop, but that doesn't really make sense. Maybe it's not
necessary?

> +    // Get the TableIndex'th bit of the bitmask.
> +    // If this bit is 0 (meaning hole) jump to the default destination,
> +    // else continue with table lookup.
> +    IntegerType *MapTy = TableMask->getType();
> +    Value *CmpVal = Builder.CreateZExtOrTrunc(TableIndex, MapTy,
> +                                              "switch.cmpval");

CmpVal doesn't seem like a great name.

> +    Constant *DV;
> +    if (NeedMask)  // To fill the holes any valid value suffices.
> +      DV = ResultLists[PHI][0].second;  // The first entry is present.
> +    else
> +      DV = DefaultResults[PHI];  // This is the proper default value.

The three previous comments are not super helpful. How about something like:

// If using a bitmask, use any value to fill the lookup table holes.
Constant *DV = NeedMask ? ResultLists[PHI][0].second : DefaultResults[PHI];