[PATCH] Distribute binary operators through SELECT_CC

Duncan Sands duncan.sands at gmail.com
Fri Jul 12 01:47:43 PDT 2013


Hi Richard,

On 28/06/13 12:49, Richard Sandiford wrote:
> This patch transforms something like:
>
>    (add (select_cc X, Y, 10, 20, CC), 10)
>
> into:
>
>    (select_cc X, Y, 20, 30, CC)
>
> but generalised for a tree of select_ccs.  It allows at most one of
> the selected values to be non-constant.
>
> If this goes in, I'd like to follow it with patches to handle unary
> operations and floating-point binary operations in the same way.
>
> The main motivation was to optimise operations on scalarised vector
> selects.  It also changed the output of the R600/alu-shift.ll test that
> Tom removed earlier.  Tom reckoned the new code was an improvement
> (but defeated the original purpose of that test).

while this looks OK (I have a few small nits to pick, see below), it's not good
that the IR level optimizers don't get this.  For general transformations of
this type, the usual is (1) teach the IR level optimizers how to do it; (2) if,
due to type legalization or whatever, this nonetheless still pops up a lot at
the codegen level then do it in codegen too.  In short, I'd like to see this be
implemented at the IR level first (in which case it should probably look through
phi nodes too).

> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> @@ -988,6 +991,75 @@ bool DAGCombiner::PromoteLoad(SDValue Op) {
>    return false;
>  }
>
> +/// Return null if N is a tree containing just constants and single-use selects.
> +/// Otherwise return a node L such that replacing L with a constant would
> +/// give such a tree.
> +static SDNode *findNonConstLeaf(SDNode *N, unsigned Depth = 0) {
> +  if (N->getOpcode() == ISD::Constant || N->getOpcode() == ISD::ConstantFP)
> +    return 0;
> +
> +  if (N->getOpcode() == ISD::SELECT_CC && N->hasOneUse() && Depth < 6) {

Please turn the recursion limit into a named constant.

> +    SDNode *A = findNonConstLeaf(N->getOperand(2).getNode(), Depth + 1);
> +    if (!A)
> +      return findNonConstLeaf(N->getOperand(3).getNode(), Depth + 1);
> +    SDNode *B = findNonConstLeaf(N->getOperand(3).getNode(), Depth + 1);
> +    if (!B)
> +      return A;

Since you always call findNonConstLeaf on both sides, this seems neater:

   SDNode *A = findNonConstLeaf(N->getOperand(2).getNode(), Depth + 1);
   SDNode *B = findNonConstLeaf(N->getOperand(3).getNode(), Depth + 1);
   if (!A)
     return B;
   if (!B)
     return A;


> +  }
> +
> +  return N;
> +}
> +
> +/// Replace every leaf of N with a binary operator like Binary, with the
> +/// leaf being operand number OpNum.  Treat Leaf as a leaf too; do not
> +/// recurse through it.
> +SDValue DAGCombiner::replaceBinary(SDNode *Binary, unsigned OpNum,
> +                                   SDValue N, SDNode *Leaf) {
> +  if (N.getNode() != Leaf && N.getOpcode() == ISD::SELECT_CC) {
> +    SDValue A = replaceBinary(Binary, OpNum, N.getOperand(2), Leaf);
> +    SDValue B = replaceBinary(Binary, OpNum, N.getOperand(3), Leaf);
> +    AddToWorkList(A.getNode());
> +    AddToWorkList(B.getNode());
> +    return DAG.getNode(ISD::SELECT_CC, SDLoc(N.getNode()), N.getValueType(),
> +                       N.getOperand(0), N.getOperand(1),
> +                       A, B, N.getOperand(4));
> +  }
> +  if (OpNum == 0)
> +    return DAG.getNode(Binary->getOpcode(), SDLoc(Binary),
> +                       Binary->getValueType(0), N, Binary->getOperand(1));
> +  else
> +    return DAG.getNode(Binary->getOpcode(), SDLoc(Binary),
> +                       Binary->getValueType(0), Binary->getOperand(0), N);
> +}
> +
> +/// If binary operator N has a constant operand, try to distribute it
> +/// through the other operand.  This should usually be a win when all the
> +/// inputs to the distributed operations are constants.  E.g.:
> +///
> +///    (add (select_cc A, B, C1, C2, T), C3)
> +///    -> (select_cc A, B, (add C1, C3), (add C2, C3), T)           [this call]
> +///    -> (select_cc A, B, C1+C3, C2+C3, T)                         [later]
> +///
> +/// It can also be a win if there is exactly one distributed operation
> +/// between a constant and a non-constant, since we then have the
> +/// chance of combining that operation further.  E.g.:
> +///
> +///    (add (select_cc A, B, (add X, C1), C2, T), C3)
> +///    -> (select_cc A, B, (add (add X, C1), C3), (add C2, C3), T)  [this call]
> +///    -> (select_cc A, B, (add C, C1+C3), C2+C3, T)                [later]
> +SDValue DAGCombiner::distributeBinary(SDNode *N) {
> +  for (unsigned I = 0; I < 2; I++) {
> +    SDValue Other = N->getOperand(I ^ 1);
> +    if (Other.getOpcode() == ISD::Constant ||
> +        Other.getOpcode() == ISD::ConstantFP) {

Since the DAG combiner should be normalizing constants onto the RHS, this loop
shouldn't be necessary.

> +      SDNode *SubN = N->getOperand(I).getNode();
> +      SDNode *Leaf = findNonConstLeaf(SubN);
> +      if (Leaf != SubN)
> +        return replaceBinary(N, I, N->getOperand(I), Leaf);
> +    }
> +  }
> +  return SDValue();
> +}
>
>  //===----------------------------------------------------------------------===//
>  //  Main DAG Combiner implementation

Ciao, Duncan.




More information about the llvm-commits mailing list