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

[Duncan Sands]

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

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