[DAGCombiner][TargetLowering] FCOPYSIGN mixed types legality

[luismarques]

Summary

This patch is an alternative approach than D66725 to the problem of handling FCOPYSIGN with mixed argument types.

As suggested by @efriedma, following the existing target lowering convention for operations with multiple types (e.g. like is done for getLoadExtAction) seemed like a fine choice a priori, compared with the hook proposed in D66725. But it seems like it actually proves to be less straightforward.

The issue is whether we want to fold away casts that ensure that the FCOPYSIGN sign arguments have the same type as the magnitude arguments. In D66725 I tried to provide a hook that had safe and sensible defaults: it doesn't assume that the target is able to handle mixed types (and thus you can't fold away the "cast"). On the other hand, if the fcopysign was going to be expanded anyway then the hook reports that mixed types are fine, since the generated code handles that fine and we avoid the unneeded extension/rounding operations.

I initially tried to provide similar semantics in this patch, but that proved misguided for several reasons. The defaults for these types of multi-type operation actions are filled in in TargetLoweringBase::initActions. When that runs the derived TargetLowering hasn't yet initialized so information about the legality of types isn't yet properly set. After playing with several approaches to provide something similar I gave up and filled in the default actions with the current semantics: the copysign is legal for all type pairs except when the sign is an f128. But it seems less than ideal to promote some current target limitations to a target-independent operation default.

The array of actions is provided in CopySignActions[MVT::LAST_VALUETYPE][MVT::LAST_VALUETYPE]. At first I thought I could make that a lot smaller by trimming the array lengths to the range of scalar floating-poing types but some targets also use this stuff with vector floating-point types (which don't have a numerical ID sequential to the scalar ones). Thinking from first principles, having a table or an algorithm that generates the same values should be equivalent. I understand the desire to be consistent with the existing code, but in this case it seems like we end up with a very sparse table, where in practice we only care to modify a few entries. That favours expressing the logic as code instead of as a pre-computed table IMHO.

Does it make sense to keep both isOperationLegal(ISD::FCOPYSIGN, Ty) and also isCopySignLegal(Ty, Ty)? Given the semantics of the operation, they should be equivalent. It seems like in the existing multi-type tables there wasn't as much of that kind of overlap? I can see them easily end up being inconsistent, even though they should return the same answer.

Is this even a reasonable mechanism to express what we want? It expresses the action (legal, expand, promote, custom...) but we are only using it for the dag combine, not for anything that affects how the FCOPYSIGN is actually lowered. With the hook alternative we moved from some more abstract "can you deal with this" to asking (at the point of the combine) the more concrete "is this combination of types legal"? But we actually only care if we should do the combine, not the exact action. Take the config setCopySignAction(MVT::f64, MVT::f32, Promote). Are we actually promoting the f32 or are we just not discarding the promotion operation that was there to begin with? As the patch is I could have put another action, as long as it wasn't legal. That sounds fishy. And so on, and so forth...

[efriedma]

This isn't quite what I was trying to get at. Whether we emit a table, or use some other underlying mechanism to provide defaults, isn't really important. The key points here are:

1. There should be a clear query for whether the operation is legal.
2. We should allow generating unsupported operations before LegalizeDAG runs.
3. LegalizeDAG should support transforming illegal operations to legal ones.

[luismarques]

In D70064#1741400, @efriedma wrote:

1. There should be a clear query for whether the operation is legal.
2. We should allow generating unsupported operations before LegalizeDAG runs.
3. LegalizeDAG should support transforming illegal operations to legal ones.

Thanks for the clarification, that makes sense. I will rework the patch.

llvm/include/llvm/CodeGen/TargetLowering.h

public:
   /// Return true if the specified store with truncation has solution on this
   /// target.
   bool isTruncStoreLegalOrCustom(EVT ValVT, EVT MemVT) const {
     return isTypeLegal(ValVT) &&
       (getTruncStoreAction(ValVT, MemVT) == Legal ||
        getTruncStoreAction(ValVT, MemVT) == Custom);
   }
 
+  LegalizeAction getCopySignAction(EVT ValVT, EVT SignVT) const {
+    unsigned ValI = (unsigned) ValVT.getSimpleVT().SimpleTy;
+    unsigned SignI = (unsigned) SignVT.getSimpleVT().SimpleTy;
+    assert(ValI < MVT::LAST_VALUETYPE && SignI < MVT::LAST_VALUETYPE &&
+           "Table isn't big enough!");
+    return CopySignActions[ValI][SignI];
+  }
+
+  bool isCopySignLegal(EVT ValVT, EVT SignVT) const {
+    return getCopySignAction(ValVT, SignVT) == Legal;
+  }
+
   /// Return how the indexed load should be treated: either it is legal, needs
   /// to be promoted to a larger size, needs to be expanded to some other code
   /// sequence, or the target has a custom expander for it.
   LegalizeAction
   getIndexedLoadAction(unsigned IdxMode, MVT VT) const {
     assert(IdxMode < ISD::LAST_INDEXED_MODE && VT.isValid() &&
            "Table isn't big enough!");
     unsigned Ty = (unsigned)VT.SimpleTy;
protected:
   /// Indicate that the specified truncating store does not work with the
   /// specified type and indicate what to do about it.
   void setTruncStoreAction(MVT ValVT, MVT MemVT,
                            LegalizeAction Action) {
     assert(ValVT.isValid() && MemVT.isValid() && "Table isn't big enough!");
     TruncStoreActions[(unsigned)ValVT.SimpleTy][MemVT.SimpleTy] = Action;
   }
 
+  void setCopySignAction(MVT ValVT, MVT SignVT,
+                         LegalizeAction Action) {
+    assert(ValVT.isValid() && SignVT.isValid() && "Table isn't big enough!");
+    unsigned ValI = (unsigned) ValVT.SimpleTy;
+    unsigned SignI = (unsigned) SignVT.SimpleTy;
+    CopySignActions[ValI][SignI] = Action;
+  }
+
   /// Indicate that the specified indexed load does or does not work with the
   /// specified type and indicate what to do abort it.
   ///
   /// NOTE: All indexed mode loads are initialized to Expand in
   /// TargetLowering.cpp
   void setIndexedLoadAction(unsigned IdxMode, MVT VT,
                             LegalizeAction Action) {
     assert(VT.isValid() && IdxMode < ISD::LAST_INDEXED_MODE &&
private:
   /// For each condition code (ISD::CondCode) keep a LegalizeAction that
   /// indicates how instruction selection should deal with the condition code.
   ///
   /// Because each CC action takes up 4 bits, we need to have the array size be
   /// large enough to fit all of the value types. This can be done by rounding
   /// up the MVT::LAST_VALUETYPE value to the next multiple of 8.
   uint32_t CondCodeActions[ISD::SETCC_INVALID][(MVT::LAST_VALUETYPE + 7) / 8];
 
+  /// For each magnitude value type and each signal value type, keep a
+  /// LegalizeAction that indicates how FCOPYSIGN is handled.
+  LegalizeAction CopySignActions[MVT::LAST_VALUETYPE][MVT::LAST_VALUETYPE];
+
   ValueTypeActionImpl ValueTypeActions;
 
 private:
   LegalizeKind getTypeConversion(LLVMContext &Context, EVT VT) const;
 
   /// Targets can specify ISD nodes that they would like PerformDAGCombine
   /// callbacks for by calling setTargetDAGCombine(), which sets a bit in this
   /// array.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (TLI.isFsqrtCheap(N0, DAG))
     return SDValue();
 
   // FSQRT nodes have flags that propagate to the created nodes.
   return buildSqrtEstimate(N0, Flags);
 }
 
 /// copysign(x, fp_extend(y)) -> copysign(x, y)
 /// copysign(x, fp_round(y)) -> copysign(x, y)
-static inline bool CanCombineFCOPYSIGN_EXTEND_ROUND(SDNode *N) {
+static inline bool CanCombineFCOPYSIGN_EXTEND_ROUND(const TargetLowering &TLI,
+                                                    SDNode *N) {
   SDValue N1 = N->getOperand(1);
   if ((N1.getOpcode() == ISD::FP_EXTEND ||
        N1.getOpcode() == ISD::FP_ROUND)) {
-    // Do not optimize out type conversion of f128 type yet.
-    // For some targets like x86_64, configuration is changed to keep one f128
-    // value in one SSE register, but instruction selection cannot handle
-    // FCOPYSIGN on SSE registers yet.
+    SDValue N0 = N->getOperand(1);
+    EVT N0VT = N0->getValueType(0);
     EVT N1VT = N1->getValueType(0);
     EVT N1Op0VT = N1->getOperand(0).getValueType();
-    return (N1VT == N1Op0VT || N1Op0VT != MVT::f128);
+    return (N1VT == N1Op0VT || TLI.isCopySignLegal(N0VT, N1Op0VT));
   }
   return false;
 }
 
 SDValue DAGCombiner::visitFCOPYSIGN(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
   bool N0CFP = isConstantFPBuildVectorOrConstantFP(N0);
if (N1.getOpcode() == ISD::FABS)
     return DAG.getNode(ISD::FABS, SDLoc(N), VT, N0);
 
   // copysign(x, copysign(y,z)) -> copysign(x, z)
   if (N1.getOpcode() == ISD::FCOPYSIGN)
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT, N0, N1.getOperand(1));
 
   // copysign(x, fp_extend(y)) -> copysign(x, y)
   // copysign(x, fp_round(y)) -> copysign(x, y)
-  if (CanCombineFCOPYSIGN_EXTEND_ROUND(N))
+  if (CanCombineFCOPYSIGN_EXTEND_ROUND(TLI, N))
     return DAG.getNode(ISD::FCOPYSIGN, SDLoc(N), VT, N0, N1.getOperand(0));
 
   return SDValue();
 }
 
 SDValue DAGCombiner::visitFPOW(SDNode *N) {
   ConstantFPSDNode *ExponentC = isConstOrConstSplatFP(N->getOperand(1));
   if (!ExponentC)

llvm/lib/CodeGen/TargetLoweringBase.cpp

void TargetLoweringBase::initActions() {
   for (MVT VT : MVT::all_valuetypes()) {
     // Default all indexed load / store to expand.
     for (unsigned IM = (unsigned)ISD::PRE_INC;
          IM != (unsigned)ISD::LAST_INDEXED_MODE; ++IM) {
       setIndexedLoadAction(IM, VT, Expand);
       setIndexedStoreAction(IM, VT, Expand);
     }
 
+    for (MVT SignTy : MVT::all_valuetypes()) {
+      LegalizeAction Action = SignTy != MVT::f128 ? Legal : Expand;
+      setCopySignAction(VT, SignTy, Action);
+    }
+
     // Most backends expect to see the node which just returns the value loaded.
     setOperationAction(ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS, VT, Expand);
 
     // These operations default to expand.
     setOperationAction(ISD::FGETSIGN, VT, Expand);
     setOperationAction(ISD::CONCAT_VECTORS, VT, Expand);
     setOperationAction(ISD::FMINNUM, VT, Expand);
     setOperationAction(ISD::FMAXNUM, VT, Expand);

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

ISD::CondCode FPCCToExtend[] = {
       ISD::SETOGT, ISD::SETOGE, ISD::SETONE, ISD::SETUEQ, ISD::SETUGT,
       ISD::SETUGE, ISD::SETULT, ISD::SETULE, ISD::SETUNE, ISD::SETGT,
       ISD::SETGE,  ISD::SETNE};
 
   ISD::NodeType FPOpToExtend[] = {
       ISD::FSIN, ISD::FCOS, ISD::FSINCOS, ISD::FPOW, ISD::FREM, ISD::FP16_TO_FP,
       ISD::FP_TO_FP16};
 
+  if (Subtarget.hasStdExtF() || Subtarget.hasStdExtD()) {
+    setCopySignAction(MVT::f32, MVT::f64, Expand);
+    setCopySignAction(MVT::f64, MVT::f32, Promote);
+  }
+
   if (Subtarget.hasStdExtF()) {
     setOperationAction(ISD::FMINNUM, MVT::f32, Legal);
     setOperationAction(ISD::FMAXNUM, MVT::f32, Legal);
     for (auto CC : FPCCToExtend)
       setCondCodeAction(CC, MVT::f32, Expand);
     setOperationAction(ISD::SELECT_CC, MVT::f32, Expand);
     setOperationAction(ISD::SELECT, MVT::f32, Custom);
     setOperationAction(ISD::BR_CC, MVT::f32, Expand);

llvm/test/CodeGen/RISCV/copysign-casts.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32I
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64I
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs -mattr=+f \
+; RUN:   -target-abi ilp32f < %s | FileCheck %s -check-prefix=RV32IF
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs -mattr=+f -mattr=+d \
+; RUN:   -target-abi ilp32d < %s | FileCheck %s -check-prefix=RV32IFD
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs -mattr=+f -mattr=+d \
+; RUN:   -target-abi lp64d < %s | FileCheck %s -check-prefix=RV64IFD
+
+; Check that DAGCombiner only folds casts into the sign argument of copysign
+; when appropriate (i.e. when it would be expanded because we don't handle mixed
+; precision magnitude and sign arguments).
+
+declare double @llvm.copysign.f64(double, double)
+declare float @llvm.copysign.f32(float, float)
+
+define double @fold_promote(double %a, float %b) nounwind {
+; RV32I-LABEL: fold_promote:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    lui a3, 524288
+; RV32I-NEXT:    and a2, a2, a3
+; RV32I-NEXT:    addi a3, a3, -1
+; RV32I-NEXT:    and a1, a1, a3
+; RV32I-NEXT:    or a1, a1, a2
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: fold_promote:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    addi a2, zero, -1
+; RV64I-NEXT:    slli a2, a2, 63
+; RV64I-NEXT:    addi a2, a2, -1
+; RV64I-NEXT:    and a0, a0, a2
+; RV64I-NEXT:    addi a2, zero, 1
+; RV64I-NEXT:    slli a2, a2, 31
+; RV64I-NEXT:    and a1, a1, a2
+; RV64I-NEXT:    slli a1, a1, 32
+; RV64I-NEXT:    or a0, a0, a1
+; RV64I-NEXT:    ret
+;
+; RV32IF-LABEL: fold_promote:
+; RV32IF:       # %bb.0:
+; RV32IF-NEXT:    addi sp, sp, -16
+; RV32IF-NEXT:    sw ra, 12(sp)
+; RV32IF-NEXT:    sw s0, 8(sp)
+; RV32IF-NEXT:    sw s1, 4(sp)
+; RV32IF-NEXT:    mv s0, a1
+; RV32IF-NEXT:    mv s1, a0
+; RV32IF-NEXT:    call __extendsfdf2
+; RV32IF-NEXT:    lui a0, 524288
+; RV32IF-NEXT:    and a1, a1, a0
+; RV32IF-NEXT:    addi a0, a0, -1
+; RV32IF-NEXT:    and a0, s0, a0
+; RV32IF-NEXT:    or a1, a0, a1
+; RV32IF-NEXT:    mv a0, s1
+; RV32IF-NEXT:    lw s1, 4(sp)
+; RV32IF-NEXT:    lw s0, 8(sp)
+; RV32IF-NEXT:    lw ra, 12(sp)
+; RV32IF-NEXT:    addi sp, sp, 16
+; RV32IF-NEXT:    ret
+;
+; RV32IFD-LABEL: fold_promote:
+; RV32IFD:       # %bb.0:
+; RV32IFD-NEXT:    fcvt.d.s ft0, fa1
+; RV32IFD-NEXT:    fsgnj.d fa0, fa0, ft0
+; RV32IFD-NEXT:    ret
+;
+; RV64IFD-LABEL: fold_promote:
+; RV64IFD:       # %bb.0:
+; RV64IFD-NEXT:    fcvt.d.s ft0, fa1
+; RV64IFD-NEXT:    fsgnj.d fa0, fa0, ft0
+; RV64IFD-NEXT:    ret
+  %c = fpext float %b to double
+  %t = call double @llvm.copysign.f64(double %a, double %c)
+  ret double %t
+}
+
+define float @fold_demote(float %a, double %b) nounwind {
+; RV32I-LABEL: fold_demote:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    lui a1, 524288
+; RV32I-NEXT:    and a2, a2, a1
+; RV32I-NEXT:    addi a1, a1, -1
+; RV32I-NEXT:    and a0, a0, a1
+; RV32I-NEXT:    or a0, a0, a2
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: fold_demote:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    lui a2, 524288
+; RV64I-NEXT:    addiw a2, a2, -1
+; RV64I-NEXT:    and a0, a0, a2
+; RV64I-NEXT:    addi a2, zero, -1
+; RV64I-NEXT:    slli a2, a2, 63
+; RV64I-NEXT:    and a1, a1, a2
+; RV64I-NEXT:    srli a1, a1, 32
+; RV64I-NEXT:    or a0, a0, a1
+; RV64I-NEXT:    ret
+;
+; RV32IF-LABEL: fold_demote:
+; RV32IF:       # %bb.0:
+; RV32IF-NEXT:    addi sp, sp, -16
+; RV32IF-NEXT:    sw ra, 12(sp)
+; RV32IF-NEXT:    fsw fs0, 8(sp)
+; RV32IF-NEXT:    fmv.s fs0, fa0
+; RV32IF-NEXT:    call __truncdfsf2
+; RV32IF-NEXT:    fsgnj.s fa0, fs0, fa0
+; RV32IF-NEXT:    flw fs0, 8(sp)
+; RV32IF-NEXT:    lw ra, 12(sp)
+; RV32IF-NEXT:    addi sp, sp, 16
+; RV32IF-NEXT:    ret
+;
+; RV32IFD-LABEL: fold_demote:
+; RV32IFD:       # %bb.0:
+; RV32IFD-NEXT:    fcvt.s.d ft0, fa1
+; RV32IFD-NEXT:    fsgnj.s fa0, fa0, ft0
+; RV32IFD-NEXT:    ret
+;
+; RV64IFD-LABEL: fold_demote:
+; RV64IFD:       # %bb.0:
+; RV64IFD-NEXT:    fcvt.s.d ft0, fa1
+; RV64IFD-NEXT:    fsgnj.s fa0, fa0, ft0
+; RV64IFD-NEXT:    ret
+  %c = fptrunc double %b to float
+  %t = call float @llvm.copysign.f32(float %a, float %c)
+  ret float %t
+}