[llvm-commits] CVS: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

[Chris Lattner]

<dag combiner part>

Another great change.


> @@ -856,6 +844,10 @@
>    ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0);
>    ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1);
>    MVT::ValueType VT = N0.getValueType();
> +
> +  // fold vector ops
> +  SDOperand FoldedVOp = SimplifyVBinOp(N);
> +  if (FoldedVOp.Val) return FoldedVOp;

I'm concerned that this adds a significant amount of control flow for  
non-vector operations.  What do you think of:

   // fold vector ops
   if (MVT::isVector(N->getValueType(VT))) {
     SDOperand FoldedVOp = SimplifyVBinOp(N);
     if (FoldedVOp.Val) return FoldedVOp;
   }

for each of these?

> @@ -1098,6 +1106,11 @@
>    SDOperand RMUL = ReassociateOps(ISD::MUL, N0, N1);
>    if (RMUL.Val != 0)
>      return RMUL;
> +
> +  // If either operand is undef, the result is undef
> +  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> +    return DAG.getNode(ISD::UNDEF, VT);
> +
>    return SDOperand();
>  }

This isn't safe for multiply.  In particular, undef*X could be well  
defined to be 0 if X is dynamically always zero.  As such, this  
should return Zero.  Note that this should return vector zero (or  
disable the xform) in the vector case.

This xform is safe for add/sub, because there is no defined value  
that (when combined with an undef) can produce a defined result.

> @@ -1162,6 +1179,11 @@
>      SDOperand Op = BuildSDIV(N);
>      if (Op.Val) return Op;
>    }
> +
> +  // If either operand is undef, the result is undef
> +  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> +    return DAG.getNode(ISD::UNDEF, VT);
> +
>    return SDOperand();
>  }

This is not safe for sdiv/udiv.  Safe xforms are:

   // undef / X -> 0
   // X / undef -> undef

If in doubt, plz check instcombine.

> @@ -1229,6 +1260,10 @@
>      return Sub;
>    }
>
> +  // If either operand is undef, the result is undef
> +  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> +    return DAG.getNode(ISD::UNDEF, VT);
> +
>    return SDOperand();
>  }

// undef % X -> 0
// X % undef -> undef

for both srem and urem.

> @@ -1283,6 +1323,10 @@
>      return DAG.getNode(ISD::SRA, N0.getValueType(), N0,
>                         DAG.getConstant(MVT::getSizeInBits 
> (N0.getValueType())-1,
>                                         TLI.getShiftAmountTy()));
> +  // If either operand is undef, the result is undef
> +  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> +    return DAG.getNode(ISD::UNDEF, VT);

mulhs/mulhu seem to be the same as mul, they should produce zero  
instead of undef.

> @@ -1336,6 +1385,10 @@
>      return DAG.getNode(N0.getOpcode(), VT, ORNode, N0.getOperand(1));
>    }
>
> +  // If either operand is undef, the result is undef
> +  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> +    return DAG.getNode(ISD::UNDEF, VT);


I think this is dead.  The only way to get into this code is if N0- 
 >opcode == N1->opcode.

> @@ -2742,6 +2807,30 @@
>    SDOperand N0 = N->getOperand(0);
>    MVT::ValueType VT = N->getValueType(0);
>
> +  // If the input is a BUILD_VECTOR with all constant elements,  
> fold this now.
> +  // Only do this before legalize, since afterward the target may  
> be depending
> +  // on the bitconvert.

Interesting.  This is a good solution for now, but maybe this argues  
for having a "target build vector", like "target constant", which  
would be unmolested by the optimizer?

> +    MVT::ValueType VT = MVT::getVectorType(DstEltVT,
> +                                           Ops.size());
> +    return DAG.getNode(ISD::BUILD_VECTOR, VT, &Ops[0], Ops.size());

This idiom occurs in several places.  Do you think it makes sense to  
have a helper method on SelectionDAG to do this?

> +SDOperand DAGCombiner::visitCONCAT_VECTORS(SDNode *N) {
> +  // TODO: Check to see if this is a CONCAT_VECTORS of a bunch of
> +  // EXTRACT_SUBVECTOR operations.  If so, and if the  
> EXTRACT_SUBVECTOR vector
> +  // inputs come from at most two distinct vectors, turn this into  
> a shuffle
> +  // node.

Also, if they come from a single vector with the right subvectors, it  
could be a noop :)

> @@ -4177,24 +4121,28 @@
>    return SDOperand();
>  }
>
> +/// SimplifyVBinOp - Visit a binary vector operation, like ADD.
> +SDOperand DAGCombiner::SimplifyVBinOp(SDNode *N) {
> +  // After legalize, the target may be depending on adds and other
> +  // binary ops to provide legal ways to construct constants or other
> +  // things. Simplifying them may result in a loss of legality.
> +  if (AfterLegalize) return SDOperand();

It would be nice if this wasn't required :(


More tomorrow.  Thanks again for tackling this Dan!

-Chris