Switch containing holes via table lookup, version 4
Hans Wennborg
hans at chromium.org
Mon Mar 10 15:03:00 PDT 2014
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];
More information about the llvm-commits
mailing list