[PATCH] D33882: [DAG] Move SelectionDAG::isCommutativeBinOp to TargetLowering.

[Justin Bogner via llvm-commits]

Simon Pilgrim via Phabricator <reviews at reviews.llvm.org> writes:
> RKSimon created this revision.
>
> This will allow commutation of target-specific DAG nodes in future patches

Sounds like a good direction. LGTM.

> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D33882
>
> Files:
>   include/llvm/CodeGen/SelectionDAG.h
>   include/llvm/Target/TargetLowering.h
>   lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>   lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>   lib/CodeGen/SelectionDAG/TargetLowering.cpp
>
> Index: lib/CodeGen/SelectionDAG/TargetLowering.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/TargetLowering.cpp
> +++ lib/CodeGen/SelectionDAG/TargetLowering.cpp
> @@ -2166,7 +2166,7 @@
>            return DAG.getSetCC(dl, VT, N0.getOperand(1), N1.getOperand(1), Cond);
>          if (N0.getOperand(1) == N1.getOperand(1))
>            return DAG.getSetCC(dl, VT, N0.getOperand(0), N1.getOperand(0), Cond);
> -        if (DAG.isCommutativeBinOp(N0.getOpcode())) {
> +        if (isCommutativeBinOp(N0.getOpcode())) {
>            // If X op Y == Y op X, try other combinations.
>            if (N0.getOperand(0) == N1.getOperand(1))
>              return DAG.getSetCC(dl, VT, N0.getOperand(1), N1.getOperand(0),
> @@ -2230,7 +2230,7 @@
>            return DAG.getSetCC(dl, VT, N0.getOperand(1),
>                                DAG.getConstant(0, dl, N0.getValueType()), Cond);
>          if (N0.getOperand(1) == N1) {
> -          if (DAG.isCommutativeBinOp(N0.getOpcode()))
> +          if (isCommutativeBinOp(N0.getOpcode()))
>              return DAG.getSetCC(dl, VT, N0.getOperand(0),
>                                  DAG.getConstant(0, dl, N0.getValueType()),
>                                  Cond);
> @@ -2257,7 +2257,7 @@
>          return DAG.getSetCC(dl, VT, N1.getOperand(1),
>                          DAG.getConstant(0, dl, N1.getValueType()), Cond);
>        if (N1.getOperand(1) == N0) {
> -        if (DAG.isCommutativeBinOp(N1.getOpcode()))
> +        if (isCommutativeBinOp(N1.getOpcode()))
>            return DAG.getSetCC(dl, VT, N1.getOperand(0),
>                            DAG.getConstant(0, dl, N1.getValueType()), Cond);
>          if (N1.getNode()->hasOneUse()) {
> Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> +++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> @@ -3883,7 +3883,7 @@
>    // fold (add Sym, c) -> Sym+c
>    if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Cst1))
>      return FoldSymbolOffset(Opcode, VT, GA, Cst2);
> -  if (isCommutativeBinOp(Opcode))
> +  if (TLI->isCommutativeBinOp(Opcode))
>      if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Cst2))
>        return FoldSymbolOffset(Opcode, VT, GA, Cst1);
>  
> @@ -4029,7 +4029,7 @@
>    ConstantFPSDNode *N2CFP = dyn_cast<ConstantFPSDNode>(N2);
>  
>    // Canonicalize constant to RHS if commutative.
> -  if (isCommutativeBinOp(Opcode)) {
> +  if (TLI->isCommutativeBinOp(Opcode)) {
>      if (N1C && !N2C) {
>        std::swap(N1C, N2C);
>        std::swap(N1, N2);
> @@ -4413,7 +4413,7 @@
>  
>    // Canonicalize an UNDEF to the RHS, even over a constant.
>    if (N1.isUndef()) {
> -    if (isCommutativeBinOp(Opcode)) {
> +    if (TLI->isCommutativeBinOp(Opcode)) {
>        std::swap(N1, N2);
>      } else {
>        switch (Opcode) {
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> @@ -1563,7 +1563,7 @@
>  
>    // If N is a commutative binary node, try commuting it to enable more
>    // sdisel CSE.
> -  if (!RV.getNode() && SelectionDAG::isCommutativeBinOp(N->getOpcode()) &&
> +  if (!RV.getNode() && TLI.isCommutativeBinOp(N->getOpcode()) &&
>        N->getNumValues() == 1) {
>      SDValue N0 = N->getOperand(0);
>      SDValue N1 = N->getOperand(1);
> Index: include/llvm/Target/TargetLowering.h
> ===================================================================
> --- include/llvm/Target/TargetLowering.h
> +++ include/llvm/Target/TargetLowering.h
> @@ -1876,6 +1876,39 @@
>      return false;
>    }
>  
> +  /// Returns true if the opcode is a commutative binary operation.
> +  virtual bool isCommutativeBinOp(unsigned Opcode) const {
> +    // FIXME: This should get its info from the td file, so that we can include
> +    // target info.
> +    switch (Opcode) {
> +    case ISD::ADD:
> +    case ISD::SMIN:
> +    case ISD::SMAX:
> +    case ISD::UMIN:
> +    case ISD::UMAX:
> +    case ISD::MUL:
> +    case ISD::MULHU:
> +    case ISD::MULHS:
> +    case ISD::SMUL_LOHI:
> +    case ISD::UMUL_LOHI:
> +    case ISD::FADD:
> +    case ISD::FMUL:
> +    case ISD::AND:
> +    case ISD::OR:
> +    case ISD::XOR:
> +    case ISD::SADDO:
> +    case ISD::UADDO:
> +    case ISD::ADDC:
> +    case ISD::ADDE:
> +    case ISD::FMINNUM:
> +    case ISD::FMAXNUM:
> +    case ISD::FMINNAN:
> +    case ISD::FMAXNAN:
> +      return true;
> +    default: return false;
> +    }
> +  }
> +
>    /// Return true if it's free to truncate a value of type FromTy to type
>    /// ToTy. e.g. On x86 it's free to truncate a i32 value in register EAX to i16
>    /// by referencing its sub-register AX.
> Index: include/llvm/CodeGen/SelectionDAG.h
> ===================================================================
> --- include/llvm/CodeGen/SelectionDAG.h
> +++ include/llvm/CodeGen/SelectionDAG.h
> @@ -1229,39 +1229,6 @@
>      AllNodes.insert(Position, AllNodes.remove(N));
>    }
>  
> -  /// Returns true if the opcode is a commutative binary operation.
> -  static bool isCommutativeBinOp(unsigned Opcode) {
> -    // FIXME: This should get its info from the td file, so that we can include
> -    // target info.
> -    switch (Opcode) {
> -    case ISD::ADD:
> -    case ISD::SMIN:
> -    case ISD::SMAX:
> -    case ISD::UMIN:
> -    case ISD::UMAX:
> -    case ISD::MUL:
> -    case ISD::MULHU:
> -    case ISD::MULHS:
> -    case ISD::SMUL_LOHI:
> -    case ISD::UMUL_LOHI:
> -    case ISD::FADD:
> -    case ISD::FMUL:
> -    case ISD::AND:
> -    case ISD::OR:
> -    case ISD::XOR:
> -    case ISD::SADDO:
> -    case ISD::UADDO:
> -    case ISD::ADDC:
> -    case ISD::ADDE:
> -    case ISD::FMINNUM:
> -    case ISD::FMAXNUM:
> -    case ISD::FMINNAN:
> -    case ISD::FMAXNAN:
> -      return true;
> -    default: return false;
> -    }
> -  }
> -
>    /// Returns an APFloat semantics tag appropriate for the given type. If VT is
>    /// a vector type, the element semantics are returned.
>    static const fltSemantics &EVTToAPFloatSemantics(EVT VT) {