[llvm-commits] [llvm] r58426 - in /llvm/trunk: include/llvm/Target/TargetLowering.h lib/CodeGen/SelectionDAG/LegalizeDAG.cpp lib/CodeGen/SelectionDAG/LegalizeTypes.h lib/CodeGen/SelectionDAG/TargetLowering.cpp lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86ISelLowering.h

[Duncan Sands]

Hi Mon Ping,

>      LegalizeAction getTypeAction(MVT VT) const {
>        if (VT.isExtended()) {
> -        if (VT.isVector()) return Expand;
> +        if (VT.isVector()) {
> +          // First try vector widening
> +          return Promote;
> +        }

is the plan to do something like what is done for arbitrary
precision integers: first promote to a type which is a power-of-two
in length, then (if that type is not legal) expand until reaching
a legal type.  In the context of vectors I guess I'm asking if you
plan to do: v10i32 -> v16i32 -> two v8i32 -> four v4i32?  Only the
last of these is a simple value type, so the above getTypeAction
logic will return "Promote" for all of them, even those you want
to split like the v16i32...  In fact aren't you going to widen
such vectors for ever (infinite loop)?

> +  /// When and were to widen is target dependent based on the cost of

When and were -> When and where

> +  /// WidenNodes - For nodes that need to be widen from one vector type to

need to be widen -> need to be widened

> +  /// another, this contains the mapping of ones we have already widen.  This

of ones we have already widen -> of those we have already widened

> +  void AddWidenOperand(SDValue From, SDValue To) {

AddWidenOperand -> AddWidenedOperand

> +  /// WidenVectorOp - Widen a vector operation in order to do the computation
> +  /// in a wider type given to a wider type given by WidenVT (e.g., v3i32 to

This sentence does not parse for me...

> +  /// v4i32).  The produced value will have the correct value for the existing
> +  /// elements but no guarantee is made about the new elements at the end of
> +  /// the vector: it may be zero, sign-extended, or garbage.  This is useful

What do zero-extension and sign-extension mean in this context?

> +  /// when we have instruction operating on an illegal vector type and we want

have instruction -> have an instruction

> +  /// to widen it to do the computation on a legal wider vector type.

In general the wider type will not be legal.  For example when the vector type
is an extended type.

> +  /// Useful 16 element vector used to pass operands for widening

16 element vector -> 16 element vector type
Missing full stop at end of line.

> +  /// LoadWidenVectorOp - Load a vector for a wider type. Returns true if

I will comment fully on the implementation once you submit it for LegalizeTypes.

> +  case ISD::CONCAT_VECTORS: {

Is this for widening?

> +        // Fall thru to expand for vector

That doesn't seem wise: if the operation is "widen" surely you have
to always widen rather than sometimes expand?  After all, users of this
node are going to expect to find a widened value in the WidenedNodes
map, but it may not be there.

> +            // Check if we have widen this node with another value
> +            std::map<SDValue, SDValue>::iterator I =
> +              WidenNodes.find(ST->getValue());

Speak of the devil!  Here is someone who expected to find a widened
node but doesn't always!  I hope this is just a temporary measure
because widening support does not yet exist for all operations...

> +  // When widen is called, it is assumed that it is more efficient to use a
> +  // wide type.  The default action is to widen to operation to a wider legal
> +  // vector type and then do the operation if it is legal by calling LegalizeOp
> +  // again.  If there is no vector equivalent, we will unroll the operation, do
> +  // it, and rebuild the vector.  If most of the operations are vectorizible to
> +  // the legal type, the resulting code will be more efficient.  If this is not
> +  // the case, the resulting code will preform badly as we end up generating
> +  // code to pack/unpack the results. It is the function that calls widen
> +  // that is responsible for seeing this doesn't happen.  For some cases, we
> +  // have decided that it is not worth widening so we just split the operation.

I don't like this roll-back strategy at all...  Do you have some examples where
it is better to split than to widen?

> +      // Promote can mean
> +      //   1) On integers, it means to promote type (e.g., i8 to i32)

"For integers, it means use the promoted integer type (e.g. i8 to i32)."

> +      //   2) For vectors, it means try to widen (e.g., v3i32 to v4i32)
"For vectors, it means use the widened vector type (e.g. v3i32 to v4i32)"

I don't think there should be any "try" about it: if the type action says
widen then it should be widened.

> +        return PromoteInteger;
> +      }
> +      else {

->      } else {

> +/// getWidenVectorType: given a vector type, returns the type to widen to

getWidenVectorType -> getWidenedVectorType

> +/// If there is no vector type that we want to widen to, returns MVT::Other

Missing full stop at end of line.
Also, I don't think this information about whether we want to widen or
not should be here.  The type action should say whether it is to be
widened or to be split.  Probably this routine should simply always
return the vector type obtained by (1) if there is a legal vector type
with the same element type but more elements, return the smallest such
vector type; (2) otherwise, round the number of elements up to the
next power of two.  By the way, if the number of elements is already
a power of two and there is no wider legal vector type, then you really
want to split not widen, so I think this routine should assert in that
case.

> +/// When and were to widen is target dependent based on the cost of

When and were -> When and where

> +  // First set operation action for all vector types to either to promote

to either to -> to either

> +  // (for widening) or expand (for scalarization). Then we will selectively

or expand (for scalarization) -> or expand (for scalarization or splitting)

> +    setOperationAction(ISD::EXTRACT_SUBVECTOR,(MVT::SimpleValueType)VT, Expand);
> +    setOperationAction(ISD::CONCAT_VECTORS,(MVT::SimpleValueType)VT, Expand);

What are these changes for?

> +/// If there is no vector type that we want to widen to, returns MVT::Other
> +/// When and were to widen is target dependent based on the cost of

As above.

> +MVT X86TargetLowering::getWidenVectorType(MVT VT) {

As I mentioned above, I think all this logic should be in the type
action, not here.

> +    /// If there is no vector type that we want to widen to, returns MVT::Other
> +    /// When and were to widen is target dependent based on the cost of

As above.

Ciao,

Duncan.