Switch containing holes via table lookup, version 5
Jasper Neumann
jn at sirrida.de
Tue Mar 11 17:12:11 PDT 2014
Hello Hans, hello folks!
> I think the patch is starting to look pretty reasonable.
Thanks!
> 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.
> Some more comments below.
> It feels like we're down to the last details now :)
Yes, slowly but steadily approaching the final version. :-)
>> + // 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.
OK. Changed.
>> + bool HasCaseResults = false;
> I thought you said you'd renamed this?
Yes, but unfortunately not as intended. Corrected.
>> + 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.
I changed the comment.
>> + Value *CmpVal = Builder.CreateZExtOrTrunc(TableIndex, MapTy,
>> + "switch.cmpval");
> CmpVal doesn't seem like a great name.
You are right. MaskIndex should be better.
>> + 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];
OK. Changed.
Hans, if you think it is OK now, please check it in.
Best regards and thank you very much for your patience
Jasper
-------------- next part --------------
Index: lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- lib/Transforms/Utils/SimplifyCFG.cpp (revision 203626)
+++ lib/Transforms/Utils/SimplifyCFG.cpp (working copy)
@@ -68,6 +68,7 @@
STATISTIC(NumBitMaps, "Number of switch instructions turned into bitmaps");
STATISTIC(NumLookupTables, "Number of switch instructions turned into lookup tables");
+STATISTIC(NumLookupTablesHoles, "Number of switch instructions turned into lookup tables (holes checked)");
STATISTIC(NumSinkCommons, "Number of common instructions sunk down to the end block");
STATISTIC(NumSpeculations, "Number of speculative executed instructions");
@@ -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
+ // 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 HasDefaultResults = false;
+ if (TableHasHoles) {
+ HasDefaultResults = GetCaseResults(SI, 0, SI->getDefaultDest(), &CommonDest,
+ DefaultResultsList, DL);
+ }
+ 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).
+ return false; // The tests are judged too costly.
+ if (!(DL && DL->fitsInLegalInteger(TableSize)))
+ return false; // The needed mask is too big.
+ }
for (size_t I = 0, E = DefaultResultsList.size(); I != E; ++I) {
PHINode *PHI = DefaultResultsList[I].first;
@@ -3786,12 +3798,55 @@
// Populate the BB that does the lookups.
Builder.SetInsertPoint(LookupBB);
+
+ if (NeedMask) {
+ // Before doing the lookup we do the hole check.
+ // The LookupBB is therefore re-purposed to do the hole check
+ // and we create a new LookupBB.
+ BasicBlock *MaskBB = LookupBB;
+ MaskBB->setName("switch.hole_check");
+ LookupBB = BasicBlock::Create(Mod.getContext(),
+ "switch.lookup",
+ CommonDest->getParent(),
+ CommonDest);
+
+ // Build bitmask; fill in a 1 bit for every case.
+ APInt MaskInt(TableSize, 0);
+ APInt One(TableSize, 1);
+ APInt Zero(TableSize, 0);
+ const ResultListTy &ResultList = ResultLists[PHIs[0]];
+ for (size_t I = 0, E = ResultList.size(); I != E; ++I) {
+ uint64_t Idx = (ResultList[I].first->getValue() -
+ MinCaseVal->getValue()).getLimitedValue();
+ MaskInt |= One << Idx;
+ }
+ ConstantInt *TableMask = ConstantInt::get(Mod.getContext(), MaskInt);
+
+ // 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 *MaskIndex = Builder.CreateZExtOrTrunc(TableIndex, MapTy,
+ "switch.maskindex");
+ Value *Shifted = Builder.CreateLShr(TableMask, MaskIndex,
+ "switch.shifted");
+ Value *LoBit = Builder.CreateTrunc(Shifted,
+ Type::getInt1Ty(Mod.getContext()),
+ "switch.lobit");
+ Builder.CreateCondBr(LoBit, LookupBB, SI->getDefaultDest());
+
+ Builder.SetInsertPoint(LookupBB);
+ AddPredecessorToBlock(SI->getDefaultDest(), MaskBB, SI->getParent());
+ }
+
bool ReturnedEarly = false;
for (size_t I = 0, E = PHIs.size(); I != E; ++I) {
PHINode *PHI = PHIs[I];
+ // If using a bitmask, use any value to fill the lookup table holes.
+ Constant *DV = NeedMask ? ResultLists[PHI][0].second : DefaultResults[PHI];
SwitchLookupTable Table(Mod, TableSize, MinCaseVal, ResultLists[PHI],
- DefaultResults[PHI], DL);
+ DV, DL);
Value *Result = Table.BuildLookup(TableIndex, Builder);
@@ -3821,6 +3876,8 @@
SI->eraseFromParent();
++NumLookupTables;
+ if (NeedMask)
+ ++NumLookupTablesHoles;
return true;
}
Index: test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll
===================================================================
--- test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll (revision 203626)
+++ test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll (working copy)
@@ -832,7 +832,7 @@
; CHECK-NOT: switch i32
}
-; This lookup table will have holes, so we cannot build it without default result.
+; This lookup table will have holes, so we need to test for the holes.
define i32 @nodefaultwithholes(i32 %c) {
entry:
switch i32 %c, label %sw.default [
@@ -853,10 +853,63 @@
ret i32 %x
; 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.
+; CHECK-NOT: switch i32
}
+; 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) {
+entry:
+ switch i32 %c, label %sw.default [
+ i32 0, label %return
+ i32 1, label %sw.bb1
+ i32 2, label %sw.bb2
+ i32 4, label %sw.bb3
+ ]
+
+sw.bb1: br label %return
+sw.bb2: br label %return
+sw.bb3: br label %return
+sw.default: call void @exit(i32 1)
+ unreachable
+return:
+ %x = phi i32 [ -1, %sw.bb3 ], [ 0, %sw.bb2 ], [ 123, %sw.bb1 ], [ 55, %entry ]
+ ret i32 %x
+
+; CHECK-LABEL: @nodefaultwithholes2(
+; CHECK: switch.hole_check:
+; CHECK-NEXT: %switch.maskindex = trunc i32 %switch.tableidx to i5
+; CHECK-NEXT: %switch.shifted = lshr i5 -9, %switch.maskindex
+; CHECK-NEXT: %switch.lobit = trunc i5 %switch.shifted to i1
+; The mask is binary 10111.
+; CHECK-NOT: switch i32
+}
+
+; We don't build lookup tables with holes for switches with less than four cases.
+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
+}
+
; We build lookup tables for switches with three or more cases.
define i32 @threecases(i32 %c) {
entry:
More information about the llvm-commits
mailing list