[DAGCombiner] Don't fold FCOPYSIGN vector sign operand casts

[luismarques]

Summary

Avoid doing the following combine for vector types:

copysign(x, fp_extend(y)) -> copysign(x, y)
copysign(x, fp_round(y)) -> copysign(x, y)

That combine seemed to impede the selection of vector instruction and cause a mess in some circumstances.

This patch was extracted from D70426, which I am abandoning. The patch also refactors the code, for clarity.

[frasercrmck]

FWIW, something this would likely help with D96028, as RVV doesn't benefit from this fpextend/fptrunc folding. We have to undo it in the custom-lowering and use a custom-node to prevent infinite loops. I was wondering if the combiner should ask the target whether it's beneficial to fold the ext/trunc or not?

[luismarques]

In D96037#2542137, @frasercrmck wrote:

FWIW, something this would likely help with D96028, as RVV doesn't benefit from this fpextend/fptrunc folding. We have to undo it in the custom-lowering and use a custom-node to prevent infinite loops. I was wondering if the combiner should ask the target whether it's beneficial to fold the ext/trunc or not?

I did propose that in the past: https://reviews.llvm.org/D66725
I may have misunderstood the review feedback I got back then (spread over a variety of patches and personal email), but that feedback led to me abandoning that patch.
I think part of the problem, at the time, was that my patches stressed too much the issues around the legality of the operation/operands, and not enough about the combine being desirable.
When I re-analyzed this issue, before submitting this patch, it did seem to me that a hook would still be beneficial, even if many of the original problems had since been addressed by other means.

[craig.topper]

LGTM

[frasercrmck]

In D96037#2542151, @luismarques wrote:

I did propose that in the past: https://reviews.llvm.org/D66725
I may have misunderstood the review feedback I got back then (spread over a variety of patches and personal email), but that feedback led to me abandoning that patch.
I think part of the problem, at the time, was that my patches stressed too much the issues around the legality of the operation/operands, and not enough about the combine being desirable.
When I re-analyzed this issue, before submitting this patch, it did seem to me that a hook would still be beneficial, even if many of the original problems had since been addressed by other means.

I see. I think the problem with implementing a hook is that it appears that no in-tree target benefits from this fcopysign optimization on vector types. So while I think it's the right idea in principle it's essentially the same as what you've got here as everyone would return false. I think this can go in as-is and a hook can come along later if a target wants it.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 }
 
 /// copysign(x, fp_extend(y)) -> copysign(x, y)
 /// copysign(x, fp_round(y)) -> copysign(x, y)
 static inline bool CanCombineFCOPYSIGN_EXTEND_ROUND(SDNode *N) {
   SDValue N1 = N->getOperand(1);
   if ((N1.getOpcode() == ISD::FP_EXTEND ||
        N1.getOpcode() == ISD::FP_ROUND)) {
+    EVT N1VT = N1->getValueType(0);
+    EVT N1Op0VT = N1->getOperand(0).getValueType();
+
+    // Always fold no-op FP casts.
+    if (N1VT == N1Op0VT)
+      return true;
+
     // 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);
+    if (N1Op0VT == MVT::f128)
+      return false;
+
+    // Avoid mismatched vector operand types, for better instruction selection.
+    if (N1Op0VT.isVector())
+      return false;
+
+    return true;
   }
   return false;
 }
 
 SDValue DAGCombiner::visitFCOPYSIGN(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
   bool N0CFP = DAG.isConstantFPBuildVectorOrConstantFP(N0);

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

 ; CHECK-NEXT:    ret
   %r = call <1 x float> @llvm.copysign.v1f32(<1 x float> %a, <1 x float> %b)
   ret <1 x float> %r
 }
 
 ; WidenVecRes mismatched
 define <1 x float> @test_copysign_v1f32_v1f64(<1 x float> %a, <1 x double> %b) #0 {
 ; CHECK-LABEL: test_copysign_v1f32_v1f64:
-; CHECK-NEXT:    fcvt s1, d1
-; CHECK-NEXT:    movi.4s v2, #128, lsl #24
-; CHECK-NEXT:    bit.16b v0, v1, v2
+; CHECK-NEXT:    fcvtn v1.2s, v1.2d
+; CHECK-NEXT:    movi.2s v2, #128, lsl #24
+; CHECK-NEXT:    bit.8b v0, v1, v2
 ; CHECK-NEXT:    ret
   %tmp0 = fptrunc <1 x double> %b to <1 x float>
   %r = call <1 x float> @llvm.copysign.v1f32(<1 x float> %a, <1 x float> %tmp0)
   ret <1 x float> %r
 }
 
 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
 
 ;============ v2f64
 
 define <2 x double> @test_copysign_v2f64_v232(<2 x double> %a, <2 x float> %b) #0 {
 ; CHECK-LABEL: test_copysign_v2f64_v232:
+; CHECK-NEXT:    fcvtl v1.2d, v1.2s
 ; CHECK-NEXT:    movi.2d v2, #0000000000000000
 ; CHECK-NEXT:    fneg.2d v2, v2
-; CHECK-NEXT:    fcvtl v1.2d, v1.2s
 ; CHECK-NEXT:    bit.16b v0, v1, v2
 ; CHECK-NEXT:    ret
   %tmp0 = fpext <2 x float> %b to <2 x double>
   %r = call <2 x double> @llvm.copysign.v2f64(<2 x double> %a, <2 x double> %tmp0)
   ret <2 x double> %r
 }
 
 define <2 x double> @test_copysign_v2f64_v2f64(<2 x double> %a, <2 x double> %b) #0 {
 ; CHECK-LABEL: test_copysign_v2f64_v2f64:
 ; CHECK-NEXT:    movi.2d v2, #0000000000000000
 ; CHECK-NEXT:    fneg.2d v2, v2
 ; CHECK-NEXT:    bit.16b v0, v1, v2
 ; CHECK-NEXT:    ret
   %r = call <2 x double> @llvm.copysign.v2f64(<2 x double> %a, <2 x double> %b)
   ret <2 x double> %r
 }
 
 declare <2 x double> @llvm.copysign.v2f64(<2 x double> %a, <2 x double> %b) #0
 
 ;============ v4f64
 
 ; SplitVecRes mismatched
 define <4 x double> @test_copysign_v4f64_v4f32(<4 x double> %a, <4 x float> %b) #0 {
 ; CHECK-LABEL: test_copysign_v4f64_v4f32:
-; CHECK-NEXT:    movi.2d v3, #0000000000000000
-; CHECK-NEXT:    fcvtl2 v4.2d, v2.4s
-; CHECK-NEXT:    fcvtl v2.2d, v2.2s
-; CHECK-NEXT:    fneg.2d v3, v3
-; CHECK-NEXT:    bit.16b v1, v4, v3
-; CHECK-NEXT:    bit.16b v0, v2, v3
+; CHECK-NEXT:    fcvtl v3.2d, v2.2s
+; CHECK-NEXT:    fcvtl2 v2.2d, v2.4s
+; CHECK-NEXT:    movi.2d v4, #0000000000000000
+; CHECK-NEXT:    fneg.2d v4, v4
+; CHECK-NEXT:    bit.16b v1, v2, v4
+; CHECK-NEXT:    bit.16b v0, v3, v4
 ; CHECK-NEXT:    ret
   %tmp0 = fpext <4 x float> %b to <4 x double>
   %r = call <4 x double> @llvm.copysign.v4f64(<4 x double> %a, <4 x double> %tmp0)
   ret <4 x double> %r
 }
 
 ; SplitVecRes same
 define <4 x double> @test_copysign_v4f64_v4f64(<4 x double> %a, <4 x double> %b) #0 {

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

; AVX-NEXT:    retq
   %2 = call <4 x float> @llvm.copysign.v4f32(<4 x float> %x, <4 x float> %1)
   ret <4 x float> %2
 }
 
 ; copysign(x, fp_extend(y)) -> copysign(x, y)
 define <4 x double> @combine_vec_fcopysign_fpext_sgn(<4 x double> %x, <4 x float> %y) {
 ; SSE-LABEL: combine_vec_fcopysign_fpext_sgn:
 ; SSE:       # %bb.0:
-; SSE-NEXT:    movaps %xmm2, %xmm3
-; SSE-NEXT:    movshdup {{.*#+}} xmm4 = xmm2[1,1,3,3]
-; SSE-NEXT:    cvtss2sd %xmm2, %xmm5
+; SSE-NEXT:    cvtps2pd %xmm2, %xmm3
 ; SSE-NEXT:    movhlps {{.*#+}} xmm2 = xmm2[1,1]
-; SSE-NEXT:    shufps {{.*#+}} xmm3 = xmm3[3,3,3,3]
-; SSE-NEXT:    movaps {{.*#+}} xmm6 = [NaN,NaN]
-; SSE-NEXT:    cvtss2sd %xmm3, %xmm3
-; SSE-NEXT:    movaps %xmm6, %xmm7
-; SSE-NEXT:    andnps %xmm3, %xmm7
-; SSE-NEXT:    movaps %xmm1, %xmm3
-; SSE-NEXT:    unpckhpd {{.*#+}} xmm3 = xmm3[1],xmm1[1]
-; SSE-NEXT:    andps %xmm6, %xmm3
-; SSE-NEXT:    orps %xmm3, %xmm7
-; SSE-NEXT:    andps %xmm6, %xmm1
-; SSE-NEXT:    cvtss2sd %xmm2, %xmm2
-; SSE-NEXT:    movaps %xmm6, %xmm3
-; SSE-NEXT:    andnps %xmm2, %xmm3
-; SSE-NEXT:    orps %xmm3, %xmm1
-; SSE-NEXT:    movlhps {{.*#+}} xmm1 = xmm1[0],xmm7[0]
-; SSE-NEXT:    movaps %xmm0, %xmm2
-; SSE-NEXT:    unpckhpd {{.*#+}} xmm2 = xmm2[1],xmm0[1]
-; SSE-NEXT:    andps %xmm6, %xmm2
-; SSE-NEXT:    xorps %xmm3, %xmm3
-; SSE-NEXT:    cvtss2sd %xmm4, %xmm3
-; SSE-NEXT:    andps %xmm6, %xmm0
-; SSE-NEXT:    andnps %xmm3, %xmm6
-; SSE-NEXT:    orps %xmm2, %xmm6
-; SSE-NEXT:    andps {{.*}}(%rip), %xmm5
+; SSE-NEXT:    cvtps2pd %xmm2, %xmm2
+; SSE-NEXT:    movaps {{.*#+}} xmm4 = [NaN,NaN]
+; SSE-NEXT:    andps %xmm4, %xmm0
+; SSE-NEXT:    movaps %xmm4, %xmm5
+; SSE-NEXT:    andnps %xmm3, %xmm5
 ; SSE-NEXT:    orps %xmm5, %xmm0
-; SSE-NEXT:    movlhps {{.*#+}} xmm0 = xmm0[0],xmm6[0]
+; SSE-NEXT:    andps %xmm4, %xmm1
+; SSE-NEXT:    andnps %xmm2, %xmm4
+; SSE-NEXT:    orps %xmm4, %xmm1
 ; SSE-NEXT:    retq
 ;
 ; AVX-LABEL: combine_vec_fcopysign_fpext_sgn:
 ; AVX:       # %bb.0:
+; AVX-NEXT:    vcvtps2pd %xmm1, %ymm1
 ; AVX-NEXT:    vbroadcastsd {{.*#+}} ymm2 = [NaN,NaN,NaN,NaN]
 ; AVX-NEXT:    vandps %ymm2, %ymm0, %ymm0
-; AVX-NEXT:    vcvtps2pd %xmm1, %ymm1
 ; AVX-NEXT:    vbroadcastsd {{.*#+}} ymm2 = [-0.0E+0,-0.0E+0,-0.0E+0,-0.0E+0]
 ; AVX-NEXT:    vandps %ymm2, %ymm1, %ymm1
 ; AVX-NEXT:    vorps %ymm1, %ymm0, %ymm0
 ; AVX-NEXT:    retq
   %1 = fpext <4 x float> %y to <4 x double>
   %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:    movlhps {{.*#+}} xmm3 = xmm3[0],xmm6[0]
-; SSE-NEXT:    shufps {{.*#+}} xmm0 = xmm0[3,3,3,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 {{.*#+}} xmm1 = xmm1[0],xmm2[0]
+; SSE-NEXT:    andpd {{.*}}(%rip), %xmm1
+; SSE-NEXT:    andpd {{.*}}(%rip), %xmm0
+; SSE-NEXT:    orpd %xmm1, %xmm0
 ; SSE-NEXT:    retq
 ;
 ; AVX-LABEL: combine_vec_fcopysign_fptrunc_sgn:
 ; AVX:       # %bb.0:
+; AVX-NEXT:    vcvtpd2ps %ymm1, %xmm1
 ; 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]
 ; AVX-NEXT:    vandpd %xmm2, %xmm1, %xmm1
 ; AVX-NEXT:    vorpd %xmm1, %xmm0, %xmm0
 ; AVX-NEXT:    vzeroupper
 ; AVX-NEXT:    retq
   %1 = fptrunc <4 x double> %y to <4 x float>
   %2 = call <4 x float> @llvm.copysign.v4f32(<4 x float> %x, <4 x float> %1)
   ret <4 x float> %2
 }
 
 declare <4 x float> @llvm.fabs.v4f32(<4 x float> %p)
 declare <4 x float> @llvm.copysign.v4f32(<4 x float> %Mag, <4 x float> %Sgn)
 declare <4 x double> @llvm.copysign.v4f64(<4 x double> %Mag, <4 x double> %Sgn)