[llvm-commits] [llvm] r70225 - in /llvm/trunk: include/llvm/CodeGen/ include/llvm/Target/ lib/CodeGen/SelectionDAG/ lib/Target/CellSPU/ lib/Target/PowerPC/ lib/Target/X86/ test/CodeGen/Generic/ test/CodeGen/X86/ utils/TableGen/
Chris Lattner
clattner at apple.com
Mon Apr 27 22:48:01 PDT 2009
On Apr 27, 2009, at 11:41 AM, Nate Begeman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=70225&view=rev
> Log:
> 2nd attempt, fixing SSE4.1 issues and implementing feedback from
> duncan.
Woot again, this is awesome work!
Most of the comments below are me bitching about int vs unsigned. I
think it is actually really important to get this right, particularly
with this change. Without being clear on that, it is easy to get
confused about what is an index into the elements of a mask (which
should be unsigned) vs what is a mask value (which is -1 or a small
integer). I think that using int and unsigned consistently is a good
way to help with this.
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/include/llvm/CodeGen/SelectionDAG.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Mon Apr 27
> 13:41:29 2009
> @@ -353,6 +353,13 @@
> SDValue getConvertRndSat(MVT VT, DebugLoc dl, SDValue Val, SDValue
> DTy,
> SDValue STy,
> SDValue Rnd, SDValue Sat, ISD::CvtCode
> Code);
> +
> + /// getVectorShuffle - Return an ISD::VECTOR_SHUFFLE node. The
> number of
> + /// elements in VT, which must be a vector type, must match the
> number of
> + /// mask elements NumElts. A negative integer mask element is
> treated as
> + /// undefined.
> + SDValue getVectorShuffle(MVT VT, DebugLoc dl, SDValue N1, SDValue
> N2,
> + const int *MaskElts);
Any reason to allow any negative? Why not require exactly -1?
>
> +class ShuffleVectorSDNode : public SDNode {
Please add a comment above this explaining what this is all about and
giving an example.
>
> + SDUse Ops[2];
> + int *Mask;
Please add a commend explaining who owns the memory for Mask. When
does it need to get deallocated (if ever)? Should this be "const
int*" or can the mask actually be modified once the node is created?
> +protected:
> + friend class SelectionDAG;
> + ShuffleVectorSDNode(MVT VT, DebugLoc dl, SDValue N1, SDValue N2,
> int *M)
If Mask is marked const, M can be too.
> +
> + void getMask(SmallVectorImpl<int> &M) const {
> + MVT VT = getValueType(0);
> + M.clear();
> + for (unsigned i = 0, e = VT.getVectorNumElements(); i != e; ++i)
> + M.push_back(Mask[i]);
> + }
> + int getMaskElt(unsigned Idx) const {
> + assert(Idx < getValueType(0).getVectorNumElements() && "Idx out
> of range!");
> + return Mask[Idx];
> + }
> +
> + bool isSplat() const { return isSplatMask(Mask, getValueType(0)); }
>
> + int getSplatIndex() const {
> + assert(isSplat() && "Cannot get splat index for non-splat!");
> + return Mask[0];
> + static bool isSplatMask(const int *Mask, MVT VT);
-> body copied from below:
> +bool ShuffleVectorSDNode::isSplatMask(const int *Mask, MVT VT) {
> + int Idx = -1;
> + for (unsigned i = 0, e = VT.getVectorNumElements(); i != e; ++i) {
> + if (Idx < 0) Idx = Mask[i];
> + if (Mask[i] >= 0 && Mask[i] != Idx)
> + return false;
> + }
> + return true;
> +}
This doesn't seem like it would recognize SHUFFLE(X,UNDEF, -1, 1,1,1)
correctly as a splat of element 1. isSplatMask will return true, but
getSplatIndex will return -1.
Maybe instead of having both methods, just have "getSplatIndex" which
would return -1 if it isn't a splat, or the index if so.
> +++ llvm/trunk/include/llvm/Target/TargetLowering.h Mon Apr 27
> 13:41:29 2009
> @@ -28,6 +28,7 @@
> #include "llvm/ADT/APFloat.h"
> #include "llvm/ADT/DenseMap.h"
> #include "llvm/ADT/SmallSet.h"
> +#include "llvm/ADT/SmallVector.h"
> #include "llvm/ADT/STLExtras.h"
> #include "llvm/CodeGen/DebugLoc.h"
> #include "llvm/Target/TargetMachine.h"
> @@ -328,7 +329,7 @@
> /// support *some* VECTOR_SHUFFLE operations, those with specific
> masks.
> /// By default, if a target supports the VECTOR_SHUFFLE node, all
> mask values
> /// are assumed to be legal.
> - virtual bool isShuffleMaskLegal(SDValue Mask, MVT VT) const {
> + virtual bool isShuffleMaskLegal(SmallVectorImpl<int> &Mask, MVT
> VT) const {
Two issues:
1. It isn't a big deal, but it doesn't seem like there is any need to
require Mask to be passed in as a small vector. It seems that just
passing in a 'const int *' is enough.
2. If you do want to pass in a smallvector, please mark it const. The
implementations of isShuffleMaskLegal should not be able to modify the
vector.
> @@ -336,9 +337,7 @@
> /// used by Targets can use this to indicate if there is a suitable
> /// VECTOR_SHUFFLE that can be used to replace a VAND with a
> constant
> /// pool entry.
> + virtual bool isVectorClearMaskLegal(SmallVectorImpl<int> &M, MVT
> VT) const {
Likewise.
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Mon Apr 27
> 13:41:29 2009
> @@ -5164,9 +5178,8 @@
> // to examine the mask.
> if (BCNumEltsChanged)
> return SDValue();
> + int Idx = cast<ShuffleVectorSDNode>(InVec)->getMaskElt(Elt);
> + int NumElems = InVec.getValueType().getVectorNumElements();
> InVec = (Idx < NumElems) ? InVec.getOperand(0) :
> InVec.getOperand(1);
> if (InVec.getOpcode() == ISD::BIT_CONVERT)
> InVec = InVec.getOperand(0);
Above this blob, the code is:
} else if (InVec.getOpcode() == ISD::VECTOR_SHUFFLE) {
Please use dyn_cast<ShuffleVectorSDNode> in the "else if", eliminating
the need for the cast<>.
I don't think that extractelement indices are always *guaranteed* to
be valid constants. If they were not, this will assert and die, which
is bad. If they are out range you can treat them like undef tho.
> @@ -5256,56 +5268,36 @@
> }
>
> // If everything is good, we can make a shuffle operation.
> - MVT IndexVT = MVT::i32;
> if (VecIn1.getNode()) {
> - SmallVector<SDValue, 8> BuildVecIndices;
> + SmallVector<int, 8> Mask;
> for (unsigned i = 0; i != NumInScalars; ++i) {
> if (N->getOperand(i).getOpcode() == ISD::UNDEF) {
> - BuildVecIndices.push_back(DAG.getUNDEF(IndexVT));
> + Mask.push_back(-1);
> continue;
> }
>
> - SDValue Extract = N->getOperand(i);
> -
> // If extracting from the first vector, just use the index
> directly.
> + SDValue Extract = N->getOperand(i);
> SDValue ExtVal = Extract.getOperand(1);
> if (Extract.getOperand(0) == VecIn1) {
> + Mask.push_back(cast<ConstantSDNode>(ExtVal)->getZExtValue());
Likewise, if the index is invalid, we should bail out of creating the
shuffle.
> @@ -5325,8 +5317,10 @@
> }
>
> SDValue DAGCombiner::visitVECTOR_SHUFFLE(SDNode *N) {
> + return SDValue();
This is an amazing optimization, but I'm not sure it is intended. :)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp Mon Apr 27
> 13:41:29 2009
>
> +/// promoteShuffle - Promote a shuffle mask of a vector VT to
> perform the
> +/// same shuffle on a vector of NVT. Must not create an illegal
> shuffle mask.
> +/// e.g. <v4i32> <0, 1, 0, 1> -> v8i16 <0, 1, 2, 3, 0, 1, 2, 3>
This comment should explain that "promote" in this instance means that
it results in a shuffle with (equal to or) more elements than the
input shuffle does. "Promote" is overloaded so much in codegen that
it might be better to pick a different word :). It is also worth
clarifying that it is up to the caller to verify that the resultant
shuffle mask will be legal, the comment makes it sound like the callee
might do it.
>
> +SDValue SelectionDAGLegalize::promoteShuffle(MVT NVT, MVT VT,
> DebugLoc dl,
> + SDValue N1, SDValue N2,
> + SmallVectorImpl<int>
> &Mask) const {
> + MVT EltVT = NVT.getVectorElementType();
> + int NumMaskElts = VT.getVectorNumElements();
> + int NumDestElts = NVT.getVectorNumElements();
Any reason not to make these unsigned?
> @@ -1705,16 +1674,21 @@
> break;
> }
> break;
> - case ISD::VECTOR_SHUFFLE:
> + case ISD::VECTOR_SHUFFLE: {
> Tmp1 = LegalizeOp(Node->getOperand(0)); // Legalize the input
> vectors,
> Tmp2 = LegalizeOp(Node->getOperand(1)); // but not the shuffle
> mask.
> - Result = DAG.UpdateNodeOperands(Result, Tmp1, Tmp2, Node-
> >getOperand(2));
> + Result = DAG.UpdateNodeOperands(Result, Tmp1, Tmp2);
> + MVT VT = Result.getValueType();
> +
> + // Copy the Mask to a local SmallVector for use wi
comment fragment.
> @@ -1725,26 +1699,21 @@
> + int NumElems = VT.getVectorNumElements();
unsigned?
> SmallVector<SDValue, 8> Ops;
> + for (int i = 0; i != NumElems; ++i) {
unsigned?
> @@ -5490,6 +5457,7 @@
>
> // FIXME: it would be far nicer to change this into
> map<SDValue,uint64_t>
> // and use a bitmask instead of a list of elements.
> + // FIXME: this doesn't treat <0, u, 0, u> for example, as a splat.
I assume that this should use your splat predicate?
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp Mon
> Apr 27 13:41:29 2009
> @@ -761,6 +761,7 @@
> SDValue &Hi) {
> // The low and high parts of the original input give four input
> vectors.
> SDValue Inputs[4];
> + ShuffleVectorSDNode *SVN = cast<ShuffleVectorSDNode>(N);
Any reason not to change SplitVecRes_VECTOR_SHUFFLE to take a
ShuffleVectorSDNode* directly and do the cast in the caller? That
makes the type an explicit part of its interface.
> @@ -1761,8 +1747,9 @@
> }
>
> SDValue DAGTypeLegalizer::WidenVecRes_VECTOR_SHUFFLE(SDNode *N) {
> + ShuffleVectorSDNode *SVN = cast<ShuffleVectorSDNode>(N);
> MVT VT = N->getValueType(0);
> - unsigned NumElts = VT.getVectorNumElements();
> + int NumElts = VT.getVectorNumElements();
unsigned?
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Mon Apr 27
> 13:41:29 2009
> @@ -1126,6 +1133,110 @@
> return SDValue(CondCodeNodes[Cond], 0);
> }
>
> +static void commuteShuffle(SDValue &N1, SDValue &N2,
> SmallVectorImpl<int> &M) {
Is this generally useful? Should this be a method on the node?
Please add a comment.
> +SDValue SelectionDAG::getVectorShuffle(MVT VT, DebugLoc dl, SDValue
> N1,
> + SDValue N2, const int *Mask) {
> + assert(N1.getValueType() == N2.getValueType() && "Invalid
> VECTOR_SHUFFLE");
> + assert(VT.isVector() && N1.getValueType().isVector() &&
> + "Vector Shuffle VTs must be a vectors");
> + assert(VT.getVectorElementType() ==
> N1.getValueType().getVectorElementType()
> + && "Vector Shuffle VTs must have same element type");
> +
> + // Canonicalize shuffle undef, undef -> undef
> + if (N1.getOpcode() == ISD::UNDEF && N2.getOpcode() == ISD::UNDEF)
> + return N1;
> +
> + // Validate that all indices in Mask are within the range of the
> elements
> + // input to the shuffle.
> + int NElts = VT.getVectorNumElements();
unsigned?
>
> + SmallVector<int, 8> MaskVec;
> + for (int i = 0; i != NElts; ++i) {
unsigned i?
>
> + if (Mask[i] >= (NElts * 2)) {
> + assert(0 && "Index out of range");
Please change this to do the test in the assert:
assert(Mask[i] < (NElts * 2) && "Index out of range");
> + // Canonicalize shuffle v, v -> v, undef
> + if (N1 == N2) {
> + N2 = getUNDEF(VT);
> + for (int i = 0; i != NElts; ++i)
unsigned i;
> + // 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) {
unsigned i;
>
> + if (MaskVec[i] >= NElts) {
> + if (N2Undef)
> + MaskVec[i] = -1;
> + else
> + AllLHS = false;
> + } else if (MaskVec[i] >= 0) {
> + AllRHS = false;
> + }
> + }
> + if (AllLHS && AllRHS)
> + return getUNDEF(VT);
> + if (AllLHS)
> + N2 = getUNDEF(VT);
How about:
> + if (AllLHS && !N2Undef)
> + N2 = getUNDEF(VT);
to speed up the common case when asking for shuffle(V,undef)
> + // 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) {
unsigned i; do you hate me yet? :)
> + FoldingSetNodeID ID;
> + SDValue Ops[2] = { N1, N2 };
> + AddNodeIDNode(ID, ISD::VECTOR_SHUFFLE, getVTList(VT), Ops, 2);
> + for (int i = 0; i != NElts; ++i)
unsigned i;
> @@ -2087,19 +2198,18 @@
> SDValue SelectionDAG::getShuffleScalarElt(const SDNode *N, unsigned
> i) {
> MVT VT = N->getValueType(0);
> DebugLoc dl = N->getDebugLoc();
> + const ShuffleVectorSDNode *SVN = cast<ShuffleVectorSDNode>(N);
If possible, this method should take a const ShuffleVectorSDNode*,
instead of SDNode*.
> + int NumElems = VT.getVectorNumElements();
unsigned
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp Mon
> Apr 27 13:41:29 2009
> @@ -2435,37 +2431,42 @@
>
> // Utility for visitShuffleVector - Returns true if the mask is mask
> starting
> // from SIndx and increasing to the element length (undefs are
> allowed).
> +static bool SequentialMask(SmallVectorImpl<int> &Mask, int SIndx) {
Should this be a static method or ShuffleVectorSDNode or no?
>
> + int MaskNumElts = Mask.size();
> + for (int i = 0; i != MaskNumElts; ++i)
unsigned i / MaskNumElts;
> void SelectionDAGLowering::visitShuffleVector(User &I) {
> + SmallVector<int, 8> Mask;
> SDValue Src1 = getValue(I.getOperand(0));
> SDValue Src2 = getValue(I.getOperand(1));
>
> + // Convert the ConstantVector mask operand into an array of ints,
> with -1
> + // representing undef values.
> + SmallVector<Constant*, 8> MaskElts;
> + cast<Constant>(I.getOperand(2))->getVectorElements(MaskElts);
> + int MaskNumElts = MaskElts.size();
> + for (int i = 0; i != MaskNumElts; ++i) {
unsigned i/MaskNumElts;
...
> int SrcNumElts = SrcVT.getVectorNumElements();
unsigned?
> + SmallVector<int, 8> MappedOps;
> for (int i = 0; i != MaskNumElts; ++i) {
unsigned?
> @@ -2541,20 +2531,19 @@
> int MaxRange[2] = {-1, -1};
>
> for (int i = 0; i != MaskNumElts; ++i) {
unsigned i
> @@ -2627,12 +2608,10 @@
> MVT PtrVT = TLI.getPointerTy();
> SmallVector<SDValue,8> Ops;
> for (int i = 0; i != MaskNumElts; ++i) {
unsigned i
> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Mon Apr 27
> 13:41:29 2009
> @@ -2825,14 +2694,15 @@
> if (ISD::isNON_EXTLoad(V2))
> return false;
>
> - unsigned NumElems = Mask->getNumOperands();
> + int NumElems = Op->getValueType(0).getVectorNumElements();
unsigned
>
> +
> if (NumElems != 2 && NumElems != 4)
> return false;
> - for (unsigned i = 0, e = NumElems/2; i != e; ++i)
> - if (!isUndefOrEqual(Mask->getOperand(i), i))
> + for (int i = 0, e = NumElems/2; i != e; ++i)
unsigned i
>
> + if (!isUndefOrEqual(Op->getMaskElt(i), i))
> return false;
> + for (int i = NumElems/2; i != NumElems; ++i)
unsigned i
> @@ -2883,34 +2730,25 @@
> }
>
> /// isZeroShuffle - Returns true if N is a VECTOR_SHUFFLE that can
> be resolved
> +/// to an zero vector.
> +/// FIXME: move to dag combiner?
How about to a method on ShuffleVectorSDNode?
>
> +static bool isZeroShuffle(ShuffleVectorSDNode *N) {
> SDValue V1 = N->getOperand(0);
> SDValue V2 = N->getOperand(1);
> + int NumElems = N->getValueType(0).getVectorNumElements();
> + for (int i = 0; i != NumElems; ++i) {
unsigned i/NumElems.
> @@ -2958,127 +2796,92 @@
>
> /// NormalizeMask - V2 is a splat, modify the mask (if needed) so
> all elements
> /// that point to V2 points to its first element.
> +static SDValue NormalizeMask(ShuffleVectorSDNode *SVOp,
> SelectionDAG &DAG) {
> + MVT VT = SVOp->getValueType(0);
> + int NumElems = VT.getVectorNumElements();
> +
>
> bool Changed = false;
> + SmallVector<int, 8> MaskVec;
> + SVOp->getMask(MaskVec);
> +
> + for (int i = 0; i != NumElems; ++i) {
unsigned i/NumElems
-Chris
More information about the llvm-commits
mailing list