[llvm-commits] [Patch] Replace switches with lookup tables (PR884)

[Hans Wennborg]

Thank you very much for the review! New patch attached. More comments welcome :)

 - Hans

On Tue, Sep 4, 2012 at 12:08 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:
> Awesome, the lack of this transform has annoyed me for a long time. It's common to have switches to transform integers in codebases in LLVM and we generate bloated slow code for them.
>
> Some high level comments:
> - The lookup tables should be marked unnamed_addr. This will allow them to be merged in case a switch was duplicated in the code due to inlining or other copying mechanisms.

Thanks! Done.

> - You're creating arrays of GEPs to constant strings. In PIC code this will add runtime relocations that have to be resolved by the dynamic linker at runtime and add two or three words of overhead per GEP to the binary. I think it's safe to ignore this for now but in long term it would be nice to turn it into one large string and an array of indices into that string.

Hmm, yes that definitely sounds like a desirable follow-up. I'll add
it among the other FIXMEs in the code.

> Pedantic comments below

My favorite kind :)

>> diff --git a/lib/Transforms/Utils/SimplifyCFG.cpp b/lib/Transforms/Utils/SimplifyCFG.cpp
>> +    // Append the result from this case to the list for each phi.
>> +    for (ResultsTy::iterator I = Results.begin(), E = Results.end(); I!=E; ++I) {
>> +      if (!ResultLists.count(I->first))
>> +          PHIs.push_back(I->first);
>
> Indentation is off.

Fixed.

>> +    SmallVector<Value*, 2> GEPIndices;
>> +    GEPIndices.push_back(ConstantInt::get(Type::getInt32Ty(Mod.getContext()),
>> +                                          0));
>> +    GEPIndices.push_back(TableIndex);
>
> This could be Value *GEPIndices[] = { Builder.getInt32(0), TableIndex };

Ah, that's much better.

>> +  // Check whether the condition value is within the case range, and branch.
>> +  Builder.SetInsertPoint(SI);
>> +  Value *IsAboveMin = Builder.CreateICmpSGE(SI->getCondition(), MinCaseVal);
>> +  Value *IsBelowMax = Builder.CreateICmpSLE(SI->getCondition(), MaxCaseVal);
>> +  Value *IsBoth = Builder.CreateAnd(IsAboveMin, IsBelowMax);
>> +  Builder.CreateCondBr(IsBoth, LookupBB, SI->getDefaultDest());
>
> The canonical IR for this is (x - MinCaseVal) ult (MaxCaseVal - MinCaseVal + 1). InstCombine will clean this up for you but I think emitting the canonical IR right away would be better.

Yes, and (x - MinCaseVal) is the TableIndex, so I'm already computing
that. I'll re-order things a little and re-use it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch_to_lookup_table4.patch
Type: application/octet-stream
Size: 18787 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120904/7a16c3f4/attachment.obj>