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

[Duncan Sands]

Hi Hans,

> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Wed Sep 26 04:34:53 2012
> @@ -3240,61 +3240,127 @@
>     return true;
>   }
>
> -/// BuildLookupTable - Build a lookup table with the contents of Results, using
> -/// DefaultResult to fill the holes in the table. If the table ends up
> -/// containing the same result in each element, set *SingleResult to that value
> -/// and return NULL.
> -static GlobalVariable *BuildLookupTable(Module &M,
> -                                        uint64_t TableSize,
> -                                        ConstantInt *Offset,
> -              const SmallVector<std::pair<ConstantInt*, Constant*>, 4>& Results,
> -                                        Constant *DefaultResult,
> -                                        Constant **SingleResult) {
> -  assert(Results.size() && "Need values to build lookup table");
> -  assert(TableSize >= Results.size() && "Table needs to hold all values");
> +namespace {
> +  /// SwitchLookupTable - This class represents a lookup table that can be used
> +  /// to replace a switch.
> +  class SwitchLookupTable {
> +  public:
> +    /// SwitchLookupTable - Create a lookup table to use as a switch replacement
> +    /// with the contents of Values, using DefaultValue to fill any holes in the
> +    /// table.
> +    SwitchLookupTable(Module &M,
> +                      uint64_t TableSize,
> +                      ConstantInt *Offset,
> +               const SmallVector<std::pair<ConstantInt*, Constant*>, 4>& Values,
> +                      Constant *DefaultValue);
> +
> +    /// BuildLookup - Build instructions with Builder to retrieve the value at
> +    /// the position given by Index in the lookup table.
> +    Value *BuildLookup(Value *Index, IRBuilder<> &Builder);
> +
> +  private:
> +    // Depending on the contents of the table, it can be represented in
> +    // different ways.
> +    enum {
> +      // For tables where each element contains the same value, we just have to
> +      // store that single value and return it for each lookup.
> +      SingleValueKind,
> +
> +      // The table is stored as an array of values. Values are retrieved by load
> +      // instructions from the table.
> +      ArrayKind
> +    } Kind;
> +
> +    // For SingleValueKind, this is the single value.
> +    Constant *SingleValue;
> +
> +    // For ArrayKind, this is the array.
> +    GlobalVariable *Array;
> +  };
> +}
> +
> +SwitchLookupTable::SwitchLookupTable(Module &M,
> +                                     uint64_t TableSize,
> +                                     ConstantInt *Offset,
> +               const SmallVector<std::pair<ConstantInt*, Constant*>, 4>& Values,
> +                                     Constant *DefaultValue) {
> +  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 (.).

...

> +/// ShouldBuildLookupTable - Determine whether a lookup table should be built
> +/// for this switch, based on the number of caes, size of the table and the
> +/// types of the results.
> +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.

> +    return true;
> +
> +  return false;
>   }
>
>   /// SwitchToLookupTable - If the switch is only used to initialize one or more
...
> @@ -3370,33 +3436,16 @@
>     }
>
>     APInt RangeSpread = MaxCaseVal->getValue() - MinCaseVal->getValue();
> -  // The table density should be at lest 40%. This is the same criterion as for
> -  // jump tables, see SelectionDAGBuilder::handleJTSwitchCase.
> -  // FIXME: Find the best cut-off.
> -  // 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.

Ciao, Duncan.