[DAGCombiner][RISCV] Avoid FCOPYSIGN folding of legalizing operand casts

[luismarques]

Summary

This patch is an alternative to D66725. It provides some minimal changes that allow solving the RISC-V fcopysign issues without being blocked on larger refactoring/restructuring changes such as those of D66725 or D70064.

It gates the DAG combine folding of fp_extend/fp_round "casts" of an fcopysign sign operand when doing so would produce an operand value with an illegal type. (The folding is still done when the original sign value already had an illegal type, as in that case we expect the fcopysign to be expanded and we avoid libcalls for the value extension/rounding).

For clarity, the patch includes the RISC-V tests that motivated the DAGCombiner changes, although those can be committed separately.

[lenary]

llvm/lib/Target/RISCV/RISCVInstrInfoD.td

234

Please can you add a comment to note that these patterns will eventually not be needed, once we improve fcopysign handling?

[luismarques]

llvm/lib/Target/RISCV/RISCVInstrInfoD.td

234

Although that was my original plan, there doesn't seem to be review consensus yet that such changes will be coming and accepted. (If only this patch was committed we would be left at a reasonable place, even if not perfect.)

[asb]

llvm/lib/Target/RISCV/RISCVInstrInfoD.td

234

I take your point about not wanting to describe future plans where there might not be consensus. I'd put a brief sentence like "These patterns can be removed if..."

[david-arm]

LGTM. The changes to DAGCombiner seem reasonable to me and I can see an improvement in the X86 code too.

llvm/lib/Target/RISCV/RISCVInstrInfoD.td

234

nit: If you still plan to go ahead with the patch could you add a brief comment as suggested?

[luismarques]

In D70426#2522193, @david-arm wrote:

LGTM. The changes to DAGCombiner seem reasonable to me and I can see an improvement in the X86 code too.

This patch was originally intended to address FCOPYSIGN issues that no longer exist (in such a significant way). It also would need some tweaks when rebasing to avoid codegen regressions on new test cases, ones that arguably wouldn't be very clean. Furthermore, I believe @efriedma had argued that these legalization issues shouldn't be handled in the combiner. So, I've extracted the beneficial changes into a separate patch, D96037, and added you as a reviewer. I'm abandoning this patch.

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.
+    // (1) Do the combine if the new operand value type is legal or if the old
+    // one already wasn't legal (we're not making it worse)
+    // (2) 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.
     EVT N1VT = N1->getValueType(0);
     EVT N1Op0VT = N1->getOperand(0).getValueType();
-    return (N1VT == N1Op0VT || N1Op0VT != MVT::f128);
+    return (N1VT == N1Op0VT ||
+            ((TLI.isTypeLegal(N1Op0VT) || !TLI.isTypeLegal(N1VT)) && // (1)
+             N1Op0VT != MVT::f128));                                 // (2)
   }
   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/Target/RISCV/RISCVInstrInfoD.td

 
 def : Pat<(fsqrt FPR64:$rs1), (FSQRT_D FPR64:$rs1, 0b111)>;
 
 def : Pat<(fneg FPR64:$rs1), (FSGNJN_D $rs1, $rs1)>;
 def : Pat<(fabs FPR64:$rs1), (FSGNJX_D $rs1, $rs1)>;
 
 def : PatFpr64Fpr64<fcopysign, FSGNJ_D>;
 def : Pat<(fcopysign FPR64:$rs1, (fneg FPR64:$rs2)), (FSGNJN_D $rs1, $rs2)>;
+def : Pat<(fcopysign FPR64:$rs1, FPR32:$rs2), (FSGNJ_D $rs1, (FCVT_D_S $rs2))>;
+def : Pat<(fcopysign FPR32:$rs1, FPR64:$rs2), (FSGNJ_S $rs1, (FCVT_S_D $rs2, 0b111))>;
 
 // fmadd: rs1 * rs2 + rs3
 def : Pat<(fma FPR64:$rs1, FPR64:$rs2, FPR64:$rs3),
           (FMADD_D $rs1, $rs2, $rs3, 0b111)>;
 
 // fmsub: rs1 * rs2 - rs3
 def : Pat<(fma FPR64:$rs1, FPR64:$rs2, (fneg FPR64:$rs3)),
           (FMSUB_D FPR64:$rs1, FPR64:$rs2, FPR64:$rs3, 0b111)>;

llvm/test/CodeGen/AArch64/vector-fcopysign.ll

 
 declare <1 x float> @llvm.copysign.v1f32(<1 x float> %a, <1 x float> %b) #0
 
 ;============ v1f64
 
 ; WidenVecOp #1
 define <1 x double> @test_copysign_v1f64_v1f32(<1 x double> %a, <1 x float> %b) #0 {
 ; CHECK-LABEL: test_copysign_v1f64_v1f32:
-; CHECK-NEXT:    fcvt d1, s1
+; CHECK-NEXT:    fcvtl v1.2d, v1.2s
 ; CHECK-NEXT:    movi.2d v2, #0000000000000000
 ; CHECK-NEXT:    fneg.2d v2, v2
 ; CHECK-NEXT:    bit.16b v0, v1, v2
 ; CHECK-NEXT:    ret
   %tmp0 = fpext <1 x float> %b to <1 x double>
   %r = call <1 x double> @llvm.copysign.v1f64(<1 x double> %a, <1 x double> %tmp0)
   ret <1 x double> %r
 }

 ; CHECK-NEXT:    ret
   %r = call <4 x float> @llvm.copysign.v4f32(<4 x float> %a, <4 x float> %b)
   ret <4 x float> %r
 }
 
 ; SplitVecOp #1
 define <4 x float> @test_copysign_v4f32_v4f64(<4 x float> %a, <4 x double> %b) #0 {
 ; CHECK-LABEL: test_copysign_v4f32_v4f64:
-; CHECK-NEXT:    mov s3, v0[1]
-; CHECK-NEXT:    movi.4s v4, #128, lsl #24
-; CHECK-NEXT:    fcvt s5, d1
-; CHECK-NEXT:    mov s6, v0[2]
-; CHECK-NEXT:    mov s7, v0[3]
-; CHECK-NEXT:    bit.16b v0, v5, v4
-; CHECK-NEXT:    fcvt s5, d2
-; CHECK-NEXT:    bit.16b v6, v5, v4
-; CHECK-NEXT:    mov d1, v1[1]
-; CHECK-NEXT:    fcvt s1, d1
-; CHECK-NEXT:    bit.16b v3, v1, v4
-; CHECK-NEXT:    mov d1, v2[1]
-; CHECK-NEXT:    fcvt s1, d1
-; CHECK-NEXT:    mov.s v0[1], v3[0]
-; CHECK-NEXT:    mov.s v0[2], v6[0]
-; CHECK-NEXT:    bit.16b v7, v1, v4
-; CHECK-NEXT:    mov.s v0[3], v7[0]
+; CHECK-NEXT:    fcvtn v1.2s, v1.2d
+; CHECK-NEXT:    fcvtn2 v1.4s, v2.2d
+; CHECK-NEXT:    movi.4s v2, #128, lsl #24
+; CHECK-NEXT:    bit.16b v0, v1, v2
 ; CHECK-NEXT:    ret
   %tmp0 = fptrunc <4 x double> %b to <4 x float>
   %r = call <4 x float> @llvm.copysign.v4f32(<4 x float> %a, <4 x float> %tmp0)
   ret <4 x float> %r
 }
 
 declare <4 x float> @llvm.copysign.v4f32(<4 x float> %a, <4 x float> %b) #0
 

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 \
+; RUN:   < %s | FileCheck %s -check-prefix=RV32IF-ILP32
+; 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:    fmv.x.w a2, fa0
+; RV32IF-NEXT:    lui a3, 524288
+; RV32IF-NEXT:    and a2, a2, a3
+; RV32IF-NEXT:    addi a3, a3, -1
+; RV32IF-NEXT:    and a1, a1, a3
+; RV32IF-NEXT:    or a1, a1, a2
+; RV32IF-NEXT:    ret
+;
+; RV32IF-ILP32-LABEL: fold_promote:
+; RV32IF-ILP32:       # %bb.0:
+; RV32IF-ILP32-NEXT:    lui a3, 524288
+; RV32IF-ILP32-NEXT:    and a2, a2, a3
+; RV32IF-ILP32-NEXT:    addi a3, a3, -1
+; RV32IF-ILP32-NEXT:    and a1, a1, a3
+; RV32IF-ILP32-NEXT:    or a1, a1, a2
+; RV32IF-ILP32-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
+;
+; RV32IF-ILP32-LABEL: fold_demote:
+; RV32IF-ILP32:       # %bb.0:
+; RV32IF-ILP32-NEXT:    addi sp, sp, -16
+; RV32IF-ILP32-NEXT:    sw ra, 12(sp)
+; RV32IF-ILP32-NEXT:    fmv.w.x ft0, a0
+; RV32IF-ILP32-NEXT:    fsw ft0, 8(sp)
+; RV32IF-ILP32-NEXT:    mv a0, a1
+; RV32IF-ILP32-NEXT:    mv a1, a2
+; RV32IF-ILP32-NEXT:    call __truncdfsf2
+; RV32IF-ILP32-NEXT:    fmv.w.x ft0, a0
+; RV32IF-ILP32-NEXT:    flw ft1, 8(sp)
+; RV32IF-ILP32-NEXT:    fsgnj.s ft0, ft1, ft0
+; RV32IF-ILP32-NEXT:    fmv.x.w a0, ft0
+; RV32IF-ILP32-NEXT:    lw ra, 12(sp)
+; RV32IF-ILP32-NEXT:    addi sp, sp, 16
+; RV32IF-ILP32-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
+}

llvm/test/CodeGen/X86/combine-fcopysign.ll

; AVX-NEXT:    retq
   %2 = call <4 x double> @llvm.copysign.v4f64(<4 x double> %x, <4 x double> %1)
   ret <4 x double> %2
 }
 
 ; copysign(x, fp_round(y)) -> copysign(x, y)
 define <4 x float> @combine_vec_fcopysign_fptrunc_sgn(<4 x float> %x, <4 x double> %y) {
 ; SSE-LABEL: combine_vec_fcopysign_fptrunc_sgn:
 ; SSE:       # %bb.0:
-; SSE-NEXT:    movaps %xmm0, %xmm3
-; SSE-NEXT:    unpckhpd {{.*#+}} xmm3 = xmm3[1],xmm0[1]
-; SSE-NEXT:    movaps {{.*#+}} xmm4 = [NaN,NaN,NaN,NaN]
-; SSE-NEXT:    andps %xmm4, %xmm3
-; SSE-NEXT:    cvtsd2ss %xmm2, %xmm5
-; SSE-NEXT:    movaps %xmm4, %xmm6
-; SSE-NEXT:    andnps %xmm5, %xmm6
-; SSE-NEXT:    orps %xmm3, %xmm6
-; SSE-NEXT:    movaps %xmm0, %xmm3
-; SSE-NEXT:    andps %xmm4, %xmm3
-; SSE-NEXT:    xorps %xmm5, %xmm5
-; SSE-NEXT:    cvtsd2ss %xmm1, %xmm5
-; SSE-NEXT:    movaps %xmm4, %xmm7
-; SSE-NEXT:    andnps %xmm5, %xmm7
-; SSE-NEXT:    orps %xmm7, %xmm3
-; SSE-NEXT:    movshdup {{.*#+}} xmm5 = xmm0[1,1,3,3]
-; SSE-NEXT:    andps %xmm4, %xmm5
-; SSE-NEXT:    movhlps {{.*#+}} xmm1 = xmm1[1,1]
-; SSE-NEXT:    cvtsd2ss %xmm1, %xmm1
-; SSE-NEXT:    andps {{.*}}(%rip), %xmm1
-; SSE-NEXT:    orps %xmm5, %xmm1
-; SSE-NEXT:    unpcklps {{.*#+}} xmm3 = xmm3[0],xmm1[0],xmm3[1],xmm1[1]
-; SSE-NEXT:    insertps {{.*#+}} xmm3 = xmm3[0,1],xmm6[0],xmm3[3]
-; SSE-NEXT:    shufps {{.*#+}} xmm0 = xmm0[3,1,2,3]
-; SSE-NEXT:    andps %xmm4, %xmm0
-; SSE-NEXT:    movhlps {{.*#+}} xmm2 = xmm2[1,1]
-; SSE-NEXT:    xorps %xmm1, %xmm1
-; SSE-NEXT:    cvtsd2ss %xmm2, %xmm1
-; SSE-NEXT:    andnps %xmm1, %xmm4
-; SSE-NEXT:    orps %xmm0, %xmm4
-; SSE-NEXT:    insertps {{.*#+}} xmm3 = xmm3[0,1,2],xmm4[0]
-; SSE-NEXT:    movaps %xmm3, %xmm0
+; SSE-NEXT:    cvtpd2ps        %xmm2, %xmm2
+; SSE-NEXT:    cvtpd2ps        %xmm1, %xmm1
+; SSE-NEXT:    unpcklpd        %xmm2, %xmm1    # xmm1 = xmm1[0],xmm2[0]
+; SSE-NEXT:    andpd   .LCPI11_0(%rip), %xmm1
+; SSE-NEXT:    andpd   .LCPI11_1(%rip), %xmm0
+; SSE-NEXT:    orpd    %xmm1, %xmm0
 ; SSE-NEXT:    retq
 ;
 ; AVX-LABEL: combine_vec_fcopysign_fptrunc_sgn:
 ; AVX:       # %bb.0:
 ; AVX-NEXT:    vbroadcastss {{.*#+}} xmm2 = [NaN,NaN,NaN,NaN]
 ; AVX-NEXT:    vandpd %xmm2, %xmm0, %xmm0
 ; AVX-NEXT:    vcvtpd2ps %ymm1, %xmm1
 ; AVX-NEXT:    vbroadcastss {{.*#+}} xmm2 = [-0.0E+0,-0.0E+0,-0.0E+0,-0.0E+0]