[llvm-commits] [llvm] r164682 - /llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp

[Hans Wennborg]

Thanks Duncan! I've addressed your comments in r164692.

On Wed, Sep 26, 2012 at 11:23 AM, Duncan Sands <baldrick at free.fr> wrote:
> Hi Hans,
>
>
>> +  assert(Values.size() && "Can't build lookup table without values.");
>> +  assert(TableSize >= Values.size() && "Can't fit values in table.");
>
> traditionally assert messages finish with an exclamation mark (!) rather
> than a
> full stop (.).

Done.

>> +static bool ShouldBuildLookupTable(SwitchInst *SI,
>> +                                   uint64_t TableSize) {
>> +  // 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() * 10 >= TableSize * 4)
>
>
> This should protect itself against overflow.  For example you can return
> false
> if SI-getNumCases() >= TableSize, and if TableSize >= (~0ULL) / 10.  Then
> you
> know that neither multiplication overflows.  Probably there is an even
> simpler
> way.

I agree. Your if statement looks fine, except I think it should be
getNumCases() > TableSize, not >=.

>> -  // Be careful to avoid overlow in the density computation.
>> +  // Be careful to avoid overflow when TableSize is used in
>> +  // ShouldBuildLookupTable.
>
>
> These kind of mutual dependencies (adjusting things here so the routine
> called
> a bit below doesn't overflow) are bad, I think ShouldBuildLookupTable should
> be
> fixed to just always return a sensible result.

Right, having it in ShouldBuildLookupTable is much better.