[llvm-commits] vector widening patch

[Mon Ping Wang]

On Oct 18, 2008, at 9:52 AM, Dan Gohman wrote:

>> +      // Check if we can widen the vector type
>> +      MVT WidenVT = TLI.getWidenVectorType(VT);
>> +      if (WidenVT != MVT::Other &&
>> +          TLI.getOperationAction(Op.getNode()->getOpcode(), VT) ==
>> +          TargetLowering::Expand) {
>> +        // Widen the type
>> +        WidenVectorOp(Op, WidenVT);
>> +      } else {
>
> What is the TLI.getOperationAction check checking? Isn't it sufficient
> to say that if you have a vector type that's not legal, and the target
> provides a type to widen to, that you should choose widening?
>

What it is checking for is to see if an operation with that VT can be  
widen.  If it is not, we have to scalarize the operation instead.  I'm  
making the assumption that  some operations don't support vector types  
so they should be scalarized.

>> +  case ISD::SELECT:
>> +  case ISD::SELECT_CC:
>> +  case ISD::BSWAP:
>> +  case ISD::CTPOP:
>> +  case ISD::CTTZ:
>> +  case ISD::CTLZ: {
>> +    // For now, we assume that using vectors for these operations  
>> don't
> make
>> +    // much sense so we just split it.  We return an empty result
>
> For bswap, ctpop, cttz, and ctlz, there exist machines that have
> some of these as vector instructions, so I think they should be
> able to be widened as well.
>
> For SELECT and SELECT_CC, if these end up being how vector selects
> are represented, then it definately makes sense to be able to
> widen them :-).

I can see that being useful.  I'll make that change.


>
>
>> +  // If it a vector of two, we will assume it doesn't make sense  
>> to widen
>> +  if (NElts == 2)
>> +    return MVT::Other;
>
> Shouldn't this be "<= 2"? I guess it doesn't matter immediately,
> because single-element vectors go down the ScalarizeVectorOp path,
> but these TLI query routines can sometimes be useful outside of
> legalize too.
>
> Also, here and in other places please add a period to the end of
> the sentence in the comment.
>

I made the poor assumption that we would never see a single element  
vector.  I'll make that change and fix anywhere I have made that  
assumption.  I will add a period to the end.

>> +  // We assume that we have good rules to handle loading power of  
>> two
> loads so
>> +  // if we break down the operations to power of 2 loads.  The  
>> strategy
> is to
>> +  // load the largest power of 2 that we can easily transform to a
> legal vector
>> +  // and then insert into that vector, and the cast the result  
>> into the
> legal
>> +  // vector that we want.  This avoids unnecessary stack converts.
>
> I think the "if" in the first sentance is spurious.
>
It is.

> It looks like genWidenVectorLoads is careful to never loads any
> memory beyond the elements it needs. Is that true? That's
> important to avoid touching unnecessary cache lines or pages.
> And for volatile correctness :-).
>
> On the other hand, it might be nice to check the alignment and
> volatile flags to see if it's safe to allow a full-length legal
> wide vector load, which could be a lot more efficient. Worth
> mentioning in a comment, at least :-).
>

Yes, it is careful to avoid loading any element beyond the number of  
elements it needs to and you are right that if the item is properly  
aligned, we should use the wider vector load (though we can't do it in  
a store of course) so I should mentioned it a TODO comment.

>> +  ///   isVolatile: volatile lod
>
> typo
>
>> +  // It must be true that we the widen vector type is bigger than  
>> where
>> +  // we need to store.
>
> s/ we / when /
>
>
> Can you add some regression tests that cover the basic
> vector widening functionality?
>
Definitely!


Thanks,
   -- Mon Ping