[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

[Mon Ping Wang]

Hi Duncan,

On Oct 30, 2008, at 2:38 AM, Duncan Sands wrote:

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

The intention is that if there we will only promote if there is a  
legal wider type.  Since there is no legal V16i32, we will not try to  
promote directly to that so we won't go into a infinite loop.   
Ideally, given a v10i32 with v4i32 being legal on the machine, we  
first split it down to two v4i32 and a v2i32.  The v2i32 can then be  
promoted to v4i32 and thus we end up with three v4i32.   I discuss  
this a little more below.

>> +  /// 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
>
I'll fix these syntax problems and the ones you found below.

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

I clean it up with something like
   Widen a vector operation to a legal type given by WidenVT

>> +  /// 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?
>

What I should have wrote that it may be filled with 0s, 1s, or garbage.

>> +  /// 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?
>

That slipped in.  This is needed for the vector shuffle patch.

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

What I should have done is rewrite getTypeAction in TargetLowering to  
check the TLI is possible for widening of that vector type.  At the  
time, for some strange reason, I thought that function we were calling  
that was wrapped in the class and I didn't have access to the TLI.   
This will map exactly what will be done in LegalizeTypes.

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

This is due to the issue above.  I can get rid of this case once I  
fixed it.

>
>
>> +  // 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?
>

The last sentence is incorrect.  When we decide to widen, we should  
always widen.   For operations where a vector operation doesn't exist,  
we will generate some nasty scalar code and the rebuild the vector.

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

The issue occurs for the "Extended Vectors" where we don't have a  
known vector types coming in.  ValueTypeActions.getTypeAction(VT) will  
return "promote" for this case as it doesn't have access to the TLI to  
see if there is a type we can widen to.   In LegalizeType/ 
TargetLowering getTypeAction, we should check the TLI to decide if we  
can widen and if so, return that we are widening.

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


My logic for getTypeAction is that it will get promote and then check  
the TLI to see if a legal vector type exist.  If it doesn't, it should  
return split or scalarize.  Otherwise, it will widen.  The check to  
TLI is redundant for known types   defined in ValueType as we already  
know if a legal type exist to widen to.  Any client other than  
getTypeAction shouldn't need to check for MVT::Other because once we  
are trying to widen, the type must always exist.

So if we had vector of 3 elements and there is no valid 4 element  
vector type, we should just split and not try the next power of 2 and  
then split down to 4 operations.  The rational that I see for going to  
the power of 2 is that is more likely that the split logic  (where we  
split in 1/2) has a higher chance of finding the correct legal type.   
The down side is that if we go to a power of 2, we might end up doing  
work that doesn't benefit us.  Given the example above of v10i32, if  
we promote to v16i32, we will break it down to 4 v4i32 where one of  
the v4i32 is not useful.  With what I have done, we don't do any  
better, i.e., v10xi32 to two v5xi32 to "two v3xi32 and two v2xi32"  
that ends up to four v4i32.  What would be better is to beef up the  
split logic to see what is the largest legal vector for that element  
type and try to split based on that.


>> +/// 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?
>

I don't need them anymore so I can remove it.

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


I agree.

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


Thanks for your comments!

  -- Mon Ping
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20081030/37329e76/attachment.html>