[llvm-commits] [llvm] r168970 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll

[Matt Beaumont-Gay]

On Fri, Nov 30, 2012 at 12:39 AM, Duncan Sands <baldrick at free.fr> wrote:
> Hi,
>
>
> On 30/11/12 08:21, Matt Beaumont-Gay wrote:
>>
>> Hi Evan,
>>
>> With this change, we're seeing stage2/stage3 differences in our
>> bootstrap build. I haven't looked at it in detail, but I wanted to
>> give everybody a heads-up.
>
>
>>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Thu Nov 29 20:02:42
>>> 2012
>>> @@ -3511,22 +3511,29 @@
>>>   static bool ShouldBuildLookupTable(SwitchInst *SI,
>>>                                      uint64_t TableSize,
>>>                                      const DataLayout *TD,
>>> +                                   const TargetTransformInfo *TTI,
>>>                               const SmallDenseMap<PHINode*, Type*>&
>>> ResultTypes) {
>>>     // The table density should be at least 40%. This is the same
>>> criterion as for
>>>     // jump tables, see SelectionDAGBuilder::handleJTSwitchCase.
>>>     // FIXME: Find the best cut-off.
>>>     if (SI->getNumCases() > TableSize || TableSize >= UINT64_MAX / 10)
>>>       return false; // TableSize overflowed, or mul below might overflow.
>>> -  if (SI->getNumCases() * 10 >= TableSize * 4)
>>> -    return true;
>>>
>>>     // If each table would fit in a register, we should build it anyway.
>>> +  bool AllFit = true;
>>> +  bool HasIllegalType = false;
>>>     for (SmallDenseMap<PHINode*, Type*>::const_iterator I =
>>> ResultTypes.begin(),
>>>          E = ResultTypes.end(); I != E; ++I) {
>
>
> Dense map iteration isn't deterministic, so...

Oh, of course. I was staring too hard at the actual changed lines. Thanks!

>
>
>>> -    if (!SwitchLookupTable::WouldFitInRegister(TD, TableSize,
>>> I->second))
>>> -      return false;
>>> +    Type *Ty = I->second;
>>> +    if (!TTI->getScalarTargetTransformInfo()->isTypeLegal(Ty))
>>> +      HasIllegalType = true;
>
>
> imagine that this routine is called twice, with everything the same except
> for
> the iteration order.  Both times AllFit is set to false and we break out of
> the
> loop (below).  However the first time, due to random iteration order, we set
> HasIllegalType to 'true', but the second time we don't see that illegal type
> before breaking out of the loop.
>
> Summary:
>
> First time:  AllFit == false; HasIllegalType == true;
> Second time: AllFit == false; HasIllegalType == false;
>
>
>>> +    if (!SwitchLookupTable::WouldFitInRegister(TD, TableSize, Ty)) {
>>> +      AllFit = false;
>>> +      break;
>>> +    }
>>>     }
>>> -  return true;
>>> +
>>> +  return AllFit || (!HasIllegalType && (SI->getNumCases() * 10 >=
>>> TableSize * 4));
>
>
> Then here:
>
> First time:  return false;
> Second time: return SI->getNumCases() * 10 >= TableSize * 4;
>
> So this function's return value is not deterministic.
>
> Ciao, Duncan.
>
>
>>>   }
>>>
>>>   /// SwitchToLookupTable - If the switch is only used to initialize one
>>> or more
>>> @@ -3607,7 +3614,7 @@
>>>
>>>     APInt RangeSpread = MaxCaseVal->getValue() - MinCaseVal->getValue();
>>>     uint64_t TableSize = RangeSpread.getLimitedValue() + 1;
>>> -  if (!ShouldBuildLookupTable(SI, TableSize, TD, ResultTypes))
>>> +  if (!ShouldBuildLookupTable(SI, TableSize, TD, TTI, ResultTypes))
>>>       return false;
>>>
>>>     // Create the BB that does the lookups.
>>>
>>> Modified:
>>> llvm/trunk/test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll?rev=168970&r1=168969&r2=168970&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll
>>> (original)
>>> +++ llvm/trunk/test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll
>>> Thu Nov 29 20:02:42 2012
>>> @@ -777,3 +777,29 @@
>>>   ; CHECK: switch.lookup:
>>>   ; CHECK: getelementptr inbounds [5 x i32]* @switch.table6, i32 0, i32
>>> %switch.tableidx
>>>   }
>>> +
>>> +; Don't create a table with illegal type
>>> +; rdar://12779436
>>> +define i96 @illegaltype(i32 %c) {
>>> +entry:
>>> +  switch i32 %c, label %sw.default [
>>> +    i32 42, label %return
>>> +    i32 43, label %sw.bb1
>>> +    i32 44, label %sw.bb2
>>> +    i32 45, label %sw.bb3
>>> +    i32 46, label %sw.bb4
>>> +  ]
>>> +
>>> +sw.bb1: br label %return
>>> +sw.bb2: br label %return
>>> +sw.bb3: br label %return
>>> +sw.bb4: br label %return
>>> +sw.default: br label %return
>>> +return:
>>> +  %retval.0 = phi i96 [ 15, %sw.default ], [ 27, %sw.bb4 ], [ -1,
>>> %sw.bb3 ], [ 0, %sw.bb2 ], [ 123, %sw.bb1 ], [ 55, %entry ]
>>> +  ret i96 %retval.0
>>> +
>>> +; CHECK: @illegaltype
>>> +; CHECK-NOT: @switch.table
>>> +; CHECK: switch i32 %c
>>> +}
>>>
>>>
>>> _______________________________________________
>>> 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
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits