[llvm-commits] Patch: Widening in LegalizeType

[Duncan Sands]

Hi Mon Ping,
  
> @@ -233,6 +233,11 @@
>                         JoinIntegers(Lo, Hi));
>      return DAG.getNode(ISD::BIT_CONVERT, OutVT, InOp);
>    }
> +  case WidenVector:
> +    if (OutVT.bitsEq(TLI.getWidenVectorType(InVT)))

You should be able to use NInVT rather than TLI.getWidenVectorType.
That is because TLI.getTypeToTransformTo should return the same thing
as TLI.getWidenVectorType (I think TLI.getWidenVectorType should be
eliminated; I intend to say more about this in a later email).

> +    case ISD::SCALAR_TO_VECTOR: Res = ExpandIntOp_SCALAR_TO_VECTOR(N); break;
...
> +SDValue DAGTypeLegalizer::ExpandIntOp_SCALAR_TO_VECTOR(SDNode *N) {
> +  // Create a vector sized/aligned stack slot, store the value to element #0,
> +  // then load the whole vector back out.
> +  return CreateStackStoreLoad(N->getOperand(0), N->getValueType(0));
> +}

It seems a pity to just give up and use a stack store load.  Maybe there
are some tricks...  How about using BUILD_VECTOR on the expanded pieces?

> +  case ISD::AND:
> +  case ISD::OR:
> +  case ISD::XOR:
> +  case ISD::ADD:
> +  case ISD::SUB:
> +  case ISD::FPOW:
> +  case ISD::FPOWI: 
> +  case ISD::MUL:
> +  case ISD::MULHS:
> +  case ISD::MULHU:
> +  case ISD::FADD:
> +  case ISD::FSUB:
> +  case ISD::FMUL:
> +  case ISD::SDIV:
> +  case ISD::SREM:
> +  case ISD::FDIV:
> +  case ISD::FREM:
> +  case ISD::FCOPYSIGN:
> +  case ISD::UDIV:
> +  case ISD::UREM:
> +  case ISD::BSWAP:             Res = WidenVecRes_Binary(N); break;

Can you please put these in alphabetical order.

> +SDValue DAGTypeLegalizer::WidenVecRes_Binary(SDNode *N) {
> +  // Binary op widening

Missing full stop at end of comment.

> +  MVT WidenVT = TLI.getWidenVectorType(N->getValueType(0));
> +  SDValue InOp1 = GetWidenedVector(N->getOperand(0));
> +  SDValue InOp2 = GetWidenedVector(N->getOperand(1));
> +  assert(InOp1.getValueType() == WidenVT && InOp2.getValueType() == WidenVT);
> +  return DAG.getNode(N->getOpcode(), WidenVT, InOp1, InOp2);

Please eliminate this assert.  If you want to do this kind of
checking, best to do it systematically in SetWidenedVector.
WidenVT can be obtained as the type of InOp1.

> +SDValue DAGTypeLegalizer::WidenVecRes_Convert(SDNode *N) {

This routine is identical to WidenVecRes_Unary (except for
the asserts).  These conversions are just unary operations,
so please use WidenVecRes_Unary.  See below for some more
comments.

> +SDValue DAGTypeLegalizer::WidenVecRes_Shift(SDNode *N) {
> +  // Unary op widening
> +  MVT WidenVT = TLI.getWidenVectorType(N->getValueType(0));
> +  SDValue InOp = GetWidenedVector(N->getOperand(0));
> +  assert(InOp.getValueType() == WidenVT);

I think you should drop this assertion.  It can only fail if
the original node was invalid (SelectionDAG.cpp should check
that the two types are the same - if it doesn't then that
should be fixed).  That also means that the creation of the
new node will assert if the types are different.
You can get WidenVT as the type of InOp (more efficient).

> +SDValue DAGTypeLegalizer::WidenVecRes_Unary(SDNode *N) {
> +  // Unary op widening
> +  MVT WidenVT = TLI.getWidenVectorType(N->getValueType(0));
> +  SDValue InOp = GetWidenedVector(N->getOperand(0));
> +  assert(InOp.getValueType() == WidenVT);

I think you should allow for the case when the operand and
result have different vector types.  Like FP_TO_UINT.  So
you should have a more general check here. i.e. I think you
should fold WidenVecRes_Convert into this routine.

By the way, notice a logical consequence: for this routine
to always work, you more or less have to require **that whether
a vector is widened or not, and to what width it is widened
depends only on the number of elements and not the element
type**.  So I think it is important to decide whether you
want to require this property or not.  If not, then the logic
for this routine needs to be made more complicated.  If you
do want to require this property (I think this would be wise)
then probably it should be built in fundamentally, in places
like TargetLowering.  I mean that the underlying routines
should only be passed the number of vector elements, and not
the element type - that way the above property is necessarily
satisfied.

> +SDValue DAGTypeLegalizer::WidenVecRes_BIT_CONVERT(SDNode *N) {
> +  SDValue InOp = N->getOperand(0);
> +  MVT InVT = InOp.getValueType();
> +  MVT VT = N->getValueType(0);
> +  MVT WidenVT = TLI.getWidenVectorType(VT);

Please use TLI.getTypeToTransformTo.  This keeps everything
uniform with other legalization methods.

> +  case WidenVector:
> +    if (WidenVT.bitsEq(TLI.getWidenVectorType(InVT)))

Likewise (I won't mention this again).

> +  // Otherwise, do the convert and then widen.
> +  SmallVector<SDValue, 16> Ops;
> +  if (WidenVT.getSizeInBits() % VT.getSizeInBits()) {

Isn't this test inverted?

> +    unsigned NumConcat = WidenVT.getSizeInBits() / VT.getSizeInBits();
> +    SDValue UndefVal = DAG.getNode(ISD::UNDEF, VT);
> +    Ops.push_back(SDValue(N,0));

^ Isn't this going to lead to an infinite type legalization loop?
You can just reuse the node in a different context, you have to
change it, you have to make progress towards legalization of N.

> +    SDValue CvtOp = SDValue(N,0);

Likewise.  I guess I must be confused since you surely tested this,
but I don't see how this can possibly work.  Anyway, if I'm not
confused then I think you should do the following:

case (a).  The input is a scalar (i.e. not a vector).  Try to use
scalar_to_vector to build a vector with the same bitwidth as WidenVT
out of InOp.  Return a bitconvert of this vector to the WidenVT.
This may not always be possible.  If not, you may have to go to the
stack, see case (b) below.

case (b).  The input is a vector.  Here is where an "extend
by undef" operation for vectors would be extremely useful
(analogue of scalar_to_vector) :)  Lacking that, I suggest
you create a stack temporary of the size of WidenVT, store
InOp to it, and load it out as WidenVT.  That said, there
are probably a few cases you can handle more optimally.

> +  SmallVector<SDValue, 16> NewOps(N->op_begin(), N->op_end());

Since you know the final size, you can reserve space in the
vector, to avoid reallocations.

> +SDValue DAGTypeLegalizer::WidenVecRes_CONCAT_VECTORS(SDNode *N) {
> +  // We currently support concatenating a multiple of the incoming vector. We
> +  // could easily support widening an arbitrary amount using extract/build but
> +  // I want to see a use case before we do so.

I'm not sure I understand this comment.  I guess what you are trying
to say is that v6i32 = CONCAT(v3i32, v3i32), widens to v8i32, is not
supported yet?

> +  SmallVector<SDValue, 16> Ops;

You can reserve space.

> +  Ops.push_back(SDValue(N,0));

Shouldn't you be pushing back the operands of N?!

> +  for (unsigned i = 1; i != NumConcat; ++i) {
> +    Ops.push_back(UndefVal);
> +  }

Shouldn't you be pushing back NumConcat - N->getNumOperands() - 1
copies of undef?  Also, the braces {} are redundant.

> +SDValue DAGTypeLegalizer::WidenVecRes_CONVERT_RNDSAT(SDNode *N) {

I didn't review this function.

At this point I got tired and went to bed!  I will take a look at
the rest some other time.

Ciao,

Duncan.