[llvm] r193045 - Teach simplify-cfg how to correctly create covered lookup tables for switches on iN with N >= 3.

[Chris Lattner]

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.

> +  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.

> +  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.

>     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.

-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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131020/59f22007/attachment.html>