[llvm-commits] vector widening patch

Dan Gohman gohman at apple.com
Sat Oct 18 09:52:01 PDT 2008


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

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

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

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

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

Thanks,

Dan





More information about the llvm-commits mailing list