[LegalizeDAG] Fix FCOPYSIGN expansion

[lliu0]

Summary

In expansion of FCOPYSIGN, the shift node is missing when the two operands of FCOPYSIGN are of the same size. We should always generate shift node (if the required shift bit is not zero) to put the sign bit into the right position, regardless of the size of underlying types.

[MatzeB]

LGTM, nice catch!

• In the future please submit patches with full context to ease review (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1492–1496 ↗

(On Diff #157918)

How about using this pattern here (functionality is the same):

EVT ShiftVT = IntVT;
if (SignBit.getValueSizeInBits() < ClearedSign.getValueSizeInBits()) {
  SignBit = DAG.getNode(ISD::ZERO_EXTEND, DL, MagVT, SignBit);
  ShiftVT = MagVT;
}

[lliu0]

Thanks. I have checked in a fix with your sugguested change.

[hans]

This landed shortly after the 7.0 release branch was created. Should we merge it?

[bogner]

In D49973#1185376, @hans wrote:

This landed shortly after the 7.0 release branch was created. Should we merge it?

I think we should merge this.

[hans]

In D49973#1188035, @bogner wrote:

In D49973#1185376, @hans wrote:

This landed shortly after the 7.0 release branch was created. Should we merge it?

I think we should merge this.

Merged in r339098. Thanks!

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

SDValue SelectionDAGLegalize::ExpandFCOPYSIGN(SDNode *Node) const {
   getSignAsIntValue(MagAsInt, DL, Mag);
   EVT MagVT = MagAsInt.IntValue.getValueType();
   SDValue ClearSignMask = DAG.getConstant(~MagAsInt.SignMask, DL, MagVT);
   SDValue ClearedSign = DAG.getNode(ISD::AND, DL, MagVT, MagAsInt.IntValue,
                                     ClearSignMask);
 
   // Get the signbit at the right position for MagAsInt.
   int ShiftAmount = SignAsInt.SignBit - MagAsInt.SignBit;
-  if (SignBit.getValueSizeInBits() > ClearedSign.getValueSizeInBits()) {
-    if (ShiftAmount > 0) {
-      SDValue ShiftCnst = DAG.getConstant(ShiftAmount, DL, IntVT);
-      SignBit = DAG.getNode(ISD::SRL, DL, IntVT, SignBit, ShiftCnst);
-    } else if (ShiftAmount < 0) {
-      SDValue ShiftCnst = DAG.getConstant(-ShiftAmount, DL, IntVT);
-      SignBit = DAG.getNode(ISD::SHL, DL, IntVT, SignBit, ShiftCnst);
-    }
-    SignBit = DAG.getNode(ISD::TRUNCATE, DL, MagVT, SignBit);
-  } else if (SignBit.getValueSizeInBits() < ClearedSign.getValueSizeInBits()) {
+  EVT ShiftVT = IntVT;
+  if (SignBit.getValueSizeInBits() < ClearedSign.getValueSizeInBits()) {
     SignBit = DAG.getNode(ISD::ZERO_EXTEND, DL, MagVT, SignBit);
-    if (ShiftAmount > 0) {
-      SDValue ShiftCnst = DAG.getConstant(ShiftAmount, DL, MagVT);
-      SignBit = DAG.getNode(ISD::SRL, DL, MagVT, SignBit, ShiftCnst);
-    } else if (ShiftAmount < 0) {
-      SDValue ShiftCnst = DAG.getConstant(-ShiftAmount, DL, MagVT);
-      SignBit = DAG.getNode(ISD::SHL, DL, MagVT, SignBit, ShiftCnst);
-    }
+    ShiftVT = MagVT;
+  }
+    SDValue ShiftCnst = DAG.getConstant(ShiftAmount, DL, ShiftVT);
+    SignBit = DAG.getNode(ISD::SRL, DL, ShiftVT, SignBit, ShiftCnst);
+    SDValue ShiftCnst = DAG.getConstant(-ShiftAmount, DL, ShiftVT);
+    SignBit = DAG.getNode(ISD::SHL, DL, ShiftVT, SignBit, ShiftCnst);
+  if (SignBit.getValueSizeInBits() > ClearedSign.getValueSizeInBits()) {
+    SignBit = DAG.getNode(ISD::TRUNCATE, DL, MagVT, SignBit);
   }
 
   // Store the part with the modified sign and convert back to float.
   SDValue CopiedSign = DAG.getNode(ISD::OR, DL, MagVT, ClearedSign, SignBit);
   return modifySignAsInt(MagAsInt, DL, CopiedSign);
 }
 
 SDValue SelectionDAGLegalize::ExpandFABS(SDNode *Node) const {

llvm/trunk/test/CodeGen/AArch64/fcopysign.ll

 ; RUN: llc -o - %s | FileCheck %s
 ; Check that selection dag legalization of fcopysign works in cases with
 ; different modes for the arguments.
 target triple = "aarch64--"
 
 declare fp128 @llvm.copysign.f128(fp128, fp128)
 
-@val = global double zeroinitializer, align 8
+@val_float = global float zeroinitializer, align 4
+@val_double = global double zeroinitializer, align 8
+@val_fp128 = global fp128 zeroinitializer, align 16
 
 ; CHECK-LABEL: copysign0
-; CHECK: ldr [[REG:x[0-9]+]], [x8, :lo12:val]
+; CHECK: ldr [[REG:x[0-9]+]], [x8, :lo12:val_double]
 ; CHECK: and [[ANDREG:x[0-9]+]], [[REG]], #0x8000000000000000
 ; CHECK: lsr x[[LSRREGNUM:[0-9]+]], [[ANDREG]], #56
 ; CHECK: bfxil w[[LSRREGNUM]], w{{[0-9]+}}, #0, #7
 ; CHECK: strb w[[LSRREGNUM]],
 ; CHECK: ldr q{{[0-9]+}},
 define fp128 @copysign0() {
 entry:
-  %v = load double, double* @val, align 8
+  %v = load double, double* @val_double, align 8
   %conv = fpext double %v to fp128
   %call = tail call fp128 @llvm.copysign.f128(fp128 0xL00000000000000007FFF000000000000, fp128 %conv) #2
   ret fp128 %call
 }
+
+; CHECK-LABEL: copysign1
+; CHECK-DAG: ldr [[REG:q[0-9]+]], [x8, :lo12:val_fp128]
+; CHECK-DAG: ldr [[REG:w[0-9]+]], [x8, :lo12:val_float]
+; CHECK: and [[ANDREG:w[0-9]+]], [[REG]], #0x80000000
+; CHECK: lsr w[[LSRREGNUM:[0-9]+]], [[ANDREG]], #24
+; CHECK: bfxil w[[LSRREGNUM]], w{{[0-9]+}}, #0, #7
+; CHECK: strb w[[LSRREGNUM]],
+; CHECK: ldr q{{[0-9]+}},
+define fp128@copysign1() {
+entry:
+  %v0 = load fp128, fp128* @val_fp128, align 16
+  %v1 = load float, float* @val_float, align 4
+  %conv = fpext float %v1 to fp128
+  %call = tail call fp128 @llvm.copysign.f128(fp128 %v0, fp128 %conv)
+  ret fp128 %call
+}