[llvm-commits] [Review] Integer type legalization capable of supporting non-power-of-2 value types

[Duncan Sands]

Hi Ken,

> The attached patch is an implementation of the changes to integer type
> legalization that Duncan Sands outlined [1] for supporting
> non-power-of-2 integer value types. Reviews appreciated.
> 
> It currently recalculates transformation information whenever it is
> needed for extended types and all simple integer types that are larger
> than the largest legal type, so memoizing might improve performance.
> Thoughts? Is there any better implementation option than a std::map
> keyed on the integer size?

sorry to take so long to get around to reviewing this.  Comments below.

> +  EVT getLargestLegalIntegerType() const {
> +    unsigned LargestIntReg = MVT::LAST_INTEGER_VALUETYPE;
> +    for (; RegClassForVT[LargestIntReg] == 0; --LargestIntReg)
> +      assert(LargestIntReg >= MVT::FIRST_INTEGER_VALUETYPE
> +          && "No integer registers defined!");
> +    return (MVT::SimpleValueType) LargestIntReg;
> +}

As you mentioned, this should probably be memoized.

> +
>    class ValueTypeActionImpl {
>      /// ValueTypeActions - This is a bitvector that contains two bits for each
>      /// value type, where the two bits correspond to the LegalizeAction enum.
>      /// This can be queried with "getTypeAction(VT)".
>      /// dimension by (MVT::MAX_ALLOWED_VALUETYPE/32) * 2
>      uint32_t ValueTypeActions[(MVT::MAX_ALLOWED_VALUETYPE/32)*2];

How about unifying logic for simple and extended value types by considering
ValueTypeActions to be a cache?  All elements could be initialized to some
special value (except that the *legal* types would be recorded in it as legal).
Then if the action for a simple type is the special value, you run the logic
that determines between promote/expand (using the same code as for extended
types), then record the result in ValueTypeActions.  Thus ValueTypeActions
would be the cache for simple types, and something else would be the cache
for extended types, but otherwise they would be treated exactly the same.
For that matter, it doesn't make much sense to me to have a separation between
the logic that determines whether a type is promoted/expanded/whatever and the
logic that determines what type it is promoted/expanded/whatever to.  Probably
when you need to know the action and it is not cached, then you should compute
(at the same time) both the action and the type to transform to, and cache both
of them.  In fact you could probably use the same cache for both the action and
the type, by stuffing the action in some spare bits of the type somewhere.  As
for what cache to use for extended types (as opposed to simple types, for which
of course some kind of array like ValueTypeActions should be used), I don't
think you should key on the integer size since we might want to have extended
float types some day.  Since every extended type actually contains the
corresponding LLVM type (see the EVT field LLVMTy), it could be a DenseMap from
the LLVM type to the info.

> +    const TargetLowering &TLI;
>    public:
> -    ValueTypeActionImpl() {
> +    explicit ValueTypeActionImpl(const TargetLowering &T) : TLI(T) {

I think all you use from TLI is what the largest legal type is (see below), so
how about just passing that and not TLI?

> +          // Find a register to expand to.
> +          for (int N=2; true; N *= 2) {

No need for "true" I think - does "for (int N=2; ; N *= 2) {" work?
Also, it would seem more natural for N to be unsigned here.  Finally,
why not use a shift amount instead ("Shift" say), which you increment
by 1 each time around the loop, and do RegEVT.getSizeInBits() << Shift
rather than N * RegEVT.getSizeInBits()?

> +            for (unsigned RegVT = MVT::FIRST_INTEGER_VALUETYPE;
> +                RegVT <= LargestIntReg; 
> +                ++RegVT) {

These last two lines should be indented by one more space.

> +
> +              // Skip non-registers.
> +              EVT RegEVT = (MVT::SimpleValueType)RegVT;
> +              if (!TLI.isTypeLegal(RegEVT)) {

Here you appear to use TLI, but since only simple types can be legal, and the
legality is determined by looking up in ValueTypeActions (which is where we
are!), and don't think you really need TLI here.

> +                continue;
> +              }

There was no need for curly brackets {} around this continue.

> +              unsigned ExpansionBits = N * RegEVT.getSizeInBits();

As mentioned above, this could be a left shift by a variable holding "log of N".

> +              if (ExpansionBits >= VTBits) {

How about
   if (ExpansionBits < VTBits)
     continue;
(reduces indentation)?

> +                EVT NVT;
> +                if (ExpansionBits == VTBits)  {
> +                  // This is an exact fit. Expand it.
> +                  return Expand; 
> +                } else { // ExpansionBits > VTBits
> +                  // The type needs to be promoted before it can be expanded.
> +                  return Promote;
> +                }

No need for curly brackets around single (excluding comments) lines.

> +      if (VT.isInteger() && NVT == MVT::Other) {
> +        // Integer types that must be transformed to extended types (which
> +        // cannot be constructed in computeRegisterProperties() for lack of
> +        // an LLVMContext) are marked as MVT::Other. Fall through to the 
> +        // following code as if they were extended types.
> +      } else {
> +        return NVT;
> +      }

As I mentioned above, I think you should get rid of computeRegisterProperties()
altogether, and have all logic here.  It would run when a simple or extended
value type is seen for which the answer was not previously cached.

> +      if (IsLarge) { 
> +        // Find a register to expand to.
> +        for (int N=2; true; N *= 2) {
> +          for (unsigned RegVT = MVT::FIRST_INTEGER_VALUETYPE;
> +              RegVT <= LargestIntReg; 
> +              ++RegVT) {
> +
> +            // Skip non-registers.
> +            EVT RegEVT = (MVT::SimpleValueType)RegVT;
> +            if (!isTypeLegal(RegEVT)) {
> +              continue;
> +            }
> +
> +            unsigned ExpansionBits = N * RegEVT.getSizeInBits();
> +            if (ExpansionBits >= VTBits) {
> +              EVT NVT;
> +              if (ExpansionBits == VTBits)  {
> +                // This is an exact fit. Expand once.
> +                NVT = EVT::getIntegerVT(Context, VTBits/2); 
> +              } else { // ExpansionBits > VTBits
> +                // The type needs to be promoted before it can be expanded.
> +                NVT = EVT::getIntegerVT(Context, ExpansionBits);
> +              }
> +
> +              return NVT;
> +            }
> +          }
> +        }

A bunch of my comments about the action computation apply here too.  Also, this
illustrates pretty well my point that actions and types to transform to should
probably be computed together in the same place (and cached).

> +    /// roundToSimpleIntegerType - Round this value type up to the next
> +    /// largest SimpleValueType, if one exists. If one doesn't, round up
> +    /// to the next power-of-2 type.

Please don't introduce this method - it makes it sound like simple types mean
something, but they don't really, they are just an optimization.  So I'd rather
have this logic moved to its (unique) user.

> +  // Every integer value type larger than the largest register must eventually 
> +  // be expanded to 2 (or some power of 2) registers. If the value type does
> +  // not exactly fit in a power-of-2 number of registers, it is first promoted
> +  // to an integer type that does. If it does fit in a power-of-2 number of
> +  // registers, one stage of the expansion is performed.

As I mentioned above, rather than have duplicate logic here and above for
computing who gets expanded/promoted and to what, I reckon it would be better
to unify it all.

Ciao,

Duncan.