[llvm] r193045 - Teach simplify-cfg how to correctly create covered lookup tables for switches on iN with N >= 3.
Michael Gottesman
mgottesman at apple.com
Sun Oct 20 22:23:43 PDT 2013
r193068.
On Oct 20, 2013, at 8:43 PM, Chris Lattner <clattner at apple.com> wrote:
> LGTM! Thanks Michael,
>
> -Chris
>
> On Oct 20, 2013, at 1:51 AM, Michael Gottesman <mgottesman at apple.com> wrote:
>
>> (Attached wrong patch)
>>
>> <0001-Fix-the-predecessor-removal-logic-in-r193045.patch>
>>
>> Michael
>>
>> On Oct 20, 2013, at 1:23 AM, Michael Gottesman <mgottesman at apple.com> wrote:
>>
>>>
>>> On Oct 20, 2013, at 12:47 AM, Chris Lattner <clattner at apple.com> wrote:
>>>
>>>>
>>>> On Oct 20, 2013, at 12:04 AM, Michael Gottesman <mgottesman at apple.com> wrote:
>>>>
>>>>> Author: mgottesman
>>>>> Date: Sun Oct 20 02:04:37 2013
>>>>> New Revision: 193045
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=193045&view=rev
>>>>> Log:
>>>>> Teach simplify-cfg how to correctly create covered lookup tables for switches on iN with N >= 3.
>>>>
>>>> Thanks Michael. More specifically, this is handling "fully-covered" switches. A couple of comments:
>>>>
>>>>> + // Compute the maximum table size representable by the integer type we are
>>>>> + // switching upon.
>>>>> + const unsigned CaseSize = MinCaseVal->getType()->getPrimitiveSizeInBits();
>>>>> + const uint64_t MaxTableSize = CaseSize > 63? UINT64_MAX : 1ULL << CaseSize;
>>>>
>>>> We don't generally mark local variables const like this. Please don’t.
>>>
>>> This was a last minute thing that crept in. Its gone.
>>>
>>>>
>>>>> + assert(MaxTableSize >= TableSize &&
>>>>> + "It is impossible for a switch to have more entries than the max "
>>>>> + "representable value of its input integer type's size.");
>>>>> +
>>>>> + // If we have a covered lookup table, unconditionally branch to the lookup table
>>>>> + // BB. Otherwise, check if the condition value is within the case range. If it
>>>>> + // is so, branch to the new BB. Otherwise branch to SI's default destination.
>>>>> + const bool GeneratingCoveredLookupTable = MaxTableSize == TableSize;
>>>>
>>>> This is a "fully covered" table.
>>>
>>> Ok.
>>>
>>>>
>>>>> + if (GeneratingCoveredLookupTable) {
>>>>> + Builder.CreateBr(LookupBB);
>>>>> + } else {
>>>>> + Value *Cmp = Builder.CreateICmpULT(TableIndex, ConstantInt::get(
>>>>> + MinCaseVal->getType(), TableSize));
>>>>> + Builder.CreateCondBr(Cmp, LookupBB, SI->getDefaultDest());
>>>>> + }
>>>>>
>>>>> // Populate the BB that does the lookups.
>>>>> Builder.SetInsertPoint(LookupBB);
>>>>> @@ -3772,7 +3788,13 @@ static bool SwitchToLookupTable(SwitchIn
>>>>> // Remove the switch.
>>>>> for (unsigned i = 0; i < SI->getNumSuccessors(); ++i) {
>>>>
>>>> This is not a problem in your patch, but there is no reason to evaluate getNumSuccessors() each time through the loop.
>>>
>>> Sounds good.
>>>
>>>>
>>>>> BasicBlock *Succ = SI->getSuccessor(i);
>>>>> - if (Succ == SI->getDefaultDest()) continue;
>>>>> +
>>>>> + // If we are not generating a covered lookup table, we will have a
>>>>> + // conditional branch from SI's parent BB to SI's default destination if our
>>>>> + // input value lies outside of our case range. Thus in that case leave the
>>>>> + // default destination BB as a predecessor of SI's parent BB.
>>>>> + if (Succ == SI->getDefaultDest() && !GeneratingCoveredLookupTable)
>>>>> + continue;
>>>>
>>>> This doesn't seem like the right check. If there is a switch whose default destination is "BB" and there are explicit edges to the same block, this will incorrectly remove multiple predecessors. The right patch for this is simply:
>>>>
>>>> if (GeneratingCoveredLookupTable) {
>>>> Builder.CreateBr(LookupBB);
>>>> SI->getDefaultDest()->removePredecessor(SI->getParent());
>>>> } else {
>>>> Value *Cmp = Builder.CreateICmpULT(TableIndex, ConstantInt::get(
>>>> MinCaseVal->getType(), TableSize));
>>>> Builder.CreateCondBr(Cmp, LookupBB, SI->getDefaultDest());
>>>> }
>>>>
>>>> Without this logic in the loop.
>>>
>>> Ok you are right. Removing a successor which is no longer a successor is not a good thing to do = /. (*DOH*)
>>>
>>> How does the attached patch look?
>>>
>>> <0001-Teach-simplify-cfg-how-to-correctly-create-covered-l.patch>
>>>
>>> Michael
>>>
>>>>
>>>> -Chris
>>>>
>>>>> Succ->removePredecessor(SI->getParent());
>>>>> }
>>>>> SI->eraseFromParent();
>>>>>
>>>>> Added: llvm/trunk/test/Transforms/SimplifyCFG/CoveredLookupTable.ll
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/CoveredLookupTable.ll?rev=193045&view=auto
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/SimplifyCFG/CoveredLookupTable.ll (added)
>>>>> +++ llvm/trunk/test/Transforms/SimplifyCFG/CoveredLookupTable.ll Sun Oct 20 02:04:37 2013
>>>>> @@ -0,0 +1,48 @@
>>>>> +; RUN: opt -simplifycfg -S %s | FileCheck %s
>>>>> +; rdar://15268442
>>>>> +
>>>>> +target datalayout = "e-p:64:64:64-S128-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:128:128-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
>>>>> +target triple = "x86_64-apple-darwin12.0.0"
>>>>> +
>>>>> +; CHECK-LABEL: define i3 @coveredswitch_test(
>>>>> +; CHECK: entry:
>>>>> +; CHECK-NEXT: sub i3 %input, -4
>>>>> +; CHECK-NEXT: zext i3 %switch.tableidx to i24
>>>>> +; CHECK-NEXT: mul i24 %switch.cast, 3
>>>>> +; CHECK-NEXT: lshr i24 7507338, %switch.shiftamt
>>>>> +; CHECK-NEXT: trunc i24 %switch.downshift to i3
>>>>> +; CHECK-NEXT: ret i3 %switch.masked
>>>>> +
>>>>> +define i3 @coveredswitch_test(i3 %input) {
>>>>> +entry:
>>>>> + switch i3 %input, label %bb8 [
>>>>> + i3 0, label %bb7
>>>>> + i3 1, label %bb
>>>>> + i3 2, label %bb3
>>>>> + i3 3, label %bb4
>>>>> + i3 4, label %bb5
>>>>> + i3 5, label %bb6
>>>>> + ]
>>>>> +
>>>>> +bb: ; preds = %entry
>>>>> + br label %bb8
>>>>> +
>>>>> +bb3: ; preds = %entry
>>>>> + br label %bb8
>>>>> +
>>>>> +bb4: ; preds = %entry
>>>>> + br label %bb8
>>>>> +
>>>>> +bb5: ; preds = %entry
>>>>> + br label %bb8
>>>>> +
>>>>> +bb6: ; preds = %entry
>>>>> + br label %bb8
>>>>> +
>>>>> +bb7: ; preds = %entry
>>>>> + br label %bb8
>>>>> +
>>>>> +bb8: ; preds = %bb7, %bb6, %bb5, %bb4, %bb3, %bb, %entry
>>>>> + %result = phi i3 [ 0, %bb7 ], [ 1, %bb6 ], [ 2, %bb5 ], [ 3, %bb4 ], [ 4, %bb3 ], [ 5, %bb ], [ 6, %entry ]
>>>>> + ret i3 %result
>>>>> +}
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131020/1ceb7958/attachment.html>
More information about the llvm-commits
mailing list