[llvm-commits] [llvm] r69952 - in /llvm/trunk: include/llvm/CodeGen/ include/llvm/Target/ lib/CodeGen/SelectionDAG/ lib/Target/CellSPU/ lib/Target/PowerPC/ lib/Target/X86/ test/CodeGen/X86/ utils/TableGen/

[Duncan Sands]

Hi Nate, thanks for doing this.

> +class ShuffleVectorSDNode : public SDNode {
> +  SDUse Ops[2];
> +  int *Mask;
> +protected:
> +  friend class SelectionDAG;
> +  ShuffleVectorSDNode(MVT VT, DebugLoc dl, SDValue N1, SDValue N2, int *M)
> +    : SDNode(ISD::VECTOR_SHUFFLE, dl, getSDVTList(VT)), Mask(M) {
> +    InitOperands(Ops, N1, N2);
> +  }
> +public:
> +
> +  const int * getMask() const { return Mask; }

It would be easy to go off the end of the mask when accessing mask
elements.  How about having methods to access individual mask
elements, or turn Mask into a {int *, size} pair, which accessors
that check for overflow?

> -      unsigned Idx = cast<ConstantSDNode>(InVec.getOperand(2).
> -                                          getOperand(Elt))->getZExtValue();
> -      unsigned NumElems = InVec.getOperand(2).getNumOperands();
> +      int Idx = cast<ShuffleVectorSDNode>(InVec)->getMask()[Elt];
> +      int NumElems = InVec.getValueType().getVectorNumElements();

What happens here if Idx < 0?

>  SDValue DAGCombiner::visitVECTOR_SHUFFLE(SDNode *N) {
> -  SDValue ShufMask = N->getOperand(2);
> -  unsigned NumElts = ShufMask.getNumOperands();
> +  return SDValue();
> +  
> +  MVT VT = N->getValueType(0);
> +  unsigned NumElts = VT.getVectorNumElements();

Isn't this code unreachable since you always return SDValue()?

> @@ -762,12 +769,6 @@
>      assert(N->getValueType(0).isVector() && "Wrong return type!");
>      assert(N->getNumOperands() == N->getValueType(0).getVectorNumElements() &&
>             "Wrong number of operands!");
> -    MVT EltVT = N->getValueType(0).getVectorElementType();
> -    for (SDNode::op_iterator I = N->op_begin(), E = N->op_end(); I != E; ++I)
> -      assert((I->getValueType() == EltVT ||
> -              (EltVT.isInteger() && I->getValueType().isInteger() &&
> -               EltVT.bitsLE(I->getValueType()))) &&
> -             "Wrong operand type!");

Why are you removing this assertion?

> +  // Validate that all the indices past in in Mask are within the range of 

past in in -> passed in

> +  // elements input to the shuffle.
> +  int NElts = VT.getVectorNumElements();
> +  SmallVector<int, 8> MaskVec;
> +  for (int i = 0; i != NElts; ++i) {
> +    if (Mask[i] >= (NElts * 2)) {
> +      assert(0 && "Index out of range");
> +      return SDValue();
> +    }
> +    MaskVec.push_back(Mask[i]);
> +  }

You should probably also check that any negative values are
actually equal to -1.

> +  // Canonicalize all index into lhs, -> shuffle lhs, undef
> +  // Canonicalize all index into rhs, -> shuffle rhs, undef
> +  bool AllLHS = true, AllRHS = true;
> +  bool N2Undef = N2.getOpcode() == ISD::UNDEF;
> +  for (int i = 0; i != NElts; ++i) {
> +    if (MaskVec[i] >= NElts) {
> +      if (N2Undef)
> +        MaskVec[i] = -1;
> +      else
> +        AllLHS = false;
> +    } else if (MaskVec[i] >= 0) {
> +      AllRHS = false;
> +    }

Don't you also want to do something special if N1 is undef?
Also, you can do an early exit if both AllLHS and AllRHS are false.

> +  // If Identity shuffle, or all shuffle in to undef, return that node.
> +  bool AllUndef = true;
> +  bool Identity = true;
> +  for (int i = 0; i < NElts; ++i) {
> +    if (MaskVec[i] >= 0 && MaskVec[i] != i) Identity = false;
> +    if (MaskVec[i] >= 0) AllUndef = false;
> +  }

You can do an early exit if both Identity and AllUndef are false.

> +    // Blah?
> +    Result->getTree(0)->setTransformFn(Pattern->getTree(0)->getTransformFn());

What's this bit about?

I didn't check the target specific code.  Thanks again for doing this!

Ciao,

Duncan.