[llvm-commits] Patch: generalize vector shuffle

[Duncan Sands]

Hi Mon Ping,

general comment: there is trailing whitespace on many lines.

> @@ -1324,8 +1326,7 @@
>    const VectorType *MaskTy = dyn_cast<VectorType>(Mask->getType());
>    if (!isa<Constant>(Mask) || MaskTy == 0 ||
>        MaskTy->getElementType() != Type::Int32Ty ||
> -      MaskTy->getNumElements() !=
> -      cast<VectorType>(V1->getType())->getNumElements())
> +      MaskTy->getNumElements() <= 1)

Why not allow a mask of length 1?  The DAG combiner or SelectionDAG
can simplify this of course, but I don't see why it should be illegal.

> Assert1(MV->getNumOperands() > 1,
> +            "Shufflevector mask can not have one element!", &SV);

Likewise.

> -  unsigned NumElts = cast<VectorType>(V1->getType())->getNumElements();
> +
> +  unsigned NumElts = cast<VectorType>(Mask->getType())->getNumElements();
> +  unsigned V1NumElts = cast<VectorType>(V1->getType())->getNumElements();
>    const Type *EltTy = cast<VectorType>(V1->getType())->getElementType();

How about SrcNumElts instead of V1NumElts, and MaskNumElts instead of NumElts?
Likewise in the rest of the patch: the names given to the number of elements
are not always self-explanatory.

> +//===---- LegalizeTypesUtils.cpp - Utilities for Legalization of types

Why not only do it in LegalizeTypes?

Also, I think SplitVecRes_VECTOR_SHUFFLE needs some changes, but I didn't
spot any.

> +void DAGLegalizeTypeUtils::DetermineVectorShuffleUse(SelectionDAG& CurDAG,
> +                               SDValue Mask, unsigned Start, unsigned End,
> +                               unsigned NumElements, unsigned Half,
> +                               MVT NewEltVT, SmallVector<SDValue, 16>& NewMask,
> +                               VecUse& LhsUse, VecUse& RhsUse) {

How about a comment describing this function.  I found the names LhsUse and
RhsUse not very self-explanatory.

> +  if (LLhsUse != VecUseBoth && LRhsUse != VecUseBoth) {

I have the impression that you could reduce the amount of duplicated
code by swapping Lo and Hi parts depending on what is used.  By the way,
I thought a bit about how to do this splitting, and I think what you are
doing is pretty optimal.  However I wasn't brave enough to check that you
are actually doing what I think you are doing :)  I will check the algorithm
when you have a final version.

> +    Ops.clear();

Not sure why you have Ops.clear() at the end of each branch, rather
than once after the end of the "if"...

> +    Ops.clear();
> +  }
> +}

It's also rather pointless at the end of the function!

> +void DAGTypeLegalizer::SplitVecRes_EXTRACT_SUBVECTOR(SDNode *N, SDValue &Lo,
> +                                                     SDValue &Hi) {
> +  // We only support constant index.
> +  MVT LoVT, HiVT;
> +  GetSplitDestVTs(N->getValueType(0), LoVT, HiVT);
> +  unsigned LoNumElts = LoVT.getVectorNumElements();
> +  unsigned HiNumElts = HiVT.getVectorNumElements();
> +
> +  SDValue Vec = N->getOperand(0);
> +  SDValue Idx = N->getOperand(1);
> +
> +  if (TLI.isTypeLegal(Vec.getValueType())) {

Why not just always do this?  It should work fine even if Vec is illegal.

> +    // If the incoming vector type is legal, just extract the correct subpart.
> +    ConstantSDNode *CIdx = cast<ConstantSDNode>(Idx);

You don't have to assume that Idx is constant here, you just have to do an
ADD below.

> +    Lo = DAG.getNode(ISD::EXTRACT_SUBVECTOR, LoVT, Vec, Idx);
> +    Hi = DAG.getNode(ISD::EXTRACT_SUBVECTOR, HiVT, Vec,
> +                     DAG.getConstant(CIdx->getZExtValue() + LoNumElts,
> +                                     Idx.getValueType()));

If LoVT != HiVT then there is no reason to think that these new
indices are a multiple of the lengths LoVT/HiVT, i.e. you may be
producing illegal subvectors here.  I think you should add an
assertion that LoVT == HiVT until the vector widening stuff is
complete (and splitting is no longer done for non-power-of-two
vectors).

> +  } else {

Aren't you just repeating here the work done by
SplitVecOp_EXTRACT_SUBVECTOR?

> -  case ISD::VECTOR_SHUFFLE:
> +  case ISD::VECTOR_SHUFFLE: {
> +    assert(Node->getOperand(0).getValueType().getVectorNumElements() ==
> +           Op.getValueType().getVectorNumElements() &&
> +           "Vector shuffle must be normalized");

Why must it be normalized, and who takes care of this?  Likewise below.

> -  case ISD::FCOS: {
> +  case ISD::FCOS:
> +  case ISD::CTPOP:
> +  case ISD::CTTZ:
> +  case ISD::CTLZ: {

What's this about?

> +    // Check if we need to normalize the vector shuffle

I'm not sure this belongs in LegalizeTypes.  It seems to
me that this is to do with legalizing the operation, because
it is unsupported by targets, not legalizing the types.  As
such it belongs in LegalizeDAG.  In any case, you don't
actually do anything here!

> +  // Normalize the shuffle vector since mask and vector length don't match.
> +  assert(NumElems != 1 && "Vector shuffle can not result in 1 element");

Why not allow this (and immediately simplify to something else)?

If this only produces normalized vectors, why are changes needed elsewhere
(eg: LegalizeTypes)?  At this point I am quite confused!

>    CCIfType<[v16i8, v8i16, v4i32, v2i64, v4f32, v2f64],
> -            CCAssignToReg<[XMM0,XMM1]>>,
> +            CCAssignToReg<[XMM0,XMM1,XMM2,XMM3]>>,

What's this about?

Ciao,

Duncan.