[SelectionDAG][GISel] Make LegalizeDAG lower FNEG using integer ops.

[efriedma]

Summary

Previously, if a floating-point type was legal, but FNEG wasn't legal, we would use FSUB. Instead, we should use integer ops, to preserve the semantics. (Alternatively, there's a compiler-rt call we could use, but there isn't much reason to use that.)

It turns out we actually are still using this obscure SelectionDAG codepath in a few cases: on some targets, we have "legal" floating-point types that don't actually support any floating-point operations. In particular, ARM and AArch64 are using this path.

For GlobalISel, all targets without a native FNEG used this codepath.

The implementation for SelectionDAG is pretty simple because we can reuse the infrastructure from FCOPYSIGN. For GlobalISel, it's really easy because we can just blindly emit G_XOR and let legalization of the XOR take care of the rest.

See also 9a3dc3e, the corresponding change to type legalization.

Also includes a "bonus" change to STRICT_FSUB legalization, so we can lower an FSUB_STRICT to a float libcall.

Includes the changes to both LegalizeDAG and GlobalISel so we don't have inconsistent results in the future.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46792 .

[arsenm]

GlobalISel has the same problem

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

3243

Is it worth moving this to a ExpandFNEG helper like the other ops?

[efriedma]

Refactor. Fix a regression that came up when I rebased.

[arsenm]

This also needs to be fixed for globalisel to avoid this bug appearing or not depending on other code in the function hitting the fallback

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1574

The explanation is too vague and not quite correct. It's specifically signaling nans (and also possibly denormal flushing). fsub is a canonicalizing operation, but fneg is not

[efriedma]

Do you think the globalisel change has to be done in the same patch? I guess I can try to look into it.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1574

There's also a problem for non-signalling nans: the sign of the result is unspecified. Maybe common implementations flip the sign? Not sure.

[arsenm]

In D84287#2208438, @efriedma wrote:

Do you think the globalisel change has to be done in the same patch? I guess I can try to look into it.

Yes, but it would also make sense for it to be a separate patch. I just think it’s important that they’re at least consistent in their bugs

[efriedma]

Updated with corresponding GlobalISel changes.

[efriedma]

Ping

[RKSimon]

@arsenm Any more thoughts?

llvm/lib/Target/ARM/ARMLegalizerInfo.cpp

91 ↗

(On Diff #284530)

should this be a separate fix?

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir

2 ↗

(On Diff #284530)

Worth adding a f80 case as well?

[efriedma]

llvm/lib/Target/ARM/ARMLegalizerInfo.cpp

91 ↗

(On Diff #284530)

The tests in this patch exercise this codepath. I guess it could also be tested independently; I'll look at adding a few more tests.

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir

2 ↗

(On Diff #284530)

f80 currently isn't supported by GlobalISel.

[RKSimon]

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir

2 ↗

(On Diff #284530)

ok - I was mainly interested in seeing what happened with a non-legal integer type - not a big issue though

[arsenm]

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir

2 ↗

(On Diff #284530)

It doesn't work end to end, but it should work here if you use an 80 bit value

[efriedma]

llvm/test/CodeGen/X86/GlobalISel/legalize-fneg.mir

2 ↗

(On Diff #284530)

I wrote a test, and got "LLVM ERROR: unable to legalize instruction: %1:_(s80) = G_XOR %0:_, %2:_ (in function: test_fneg_f80)".

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

private:
   void ExpandDYNAMIC_STACKALLOC(SDNode *Node,
                                 SmallVectorImpl<SDValue> &Results);
   void getSignAsIntValue(FloatSignAsInt &State, const SDLoc &DL,
                          SDValue Value) const;
   SDValue modifySignAsInt(const FloatSignAsInt &State, const SDLoc &DL,
                           SDValue NewIntValue) const;
   SDValue ExpandFCOPYSIGN(SDNode *Node) const;
   SDValue ExpandFABS(SDNode *Node) const;
+  SDValue ExpandFNEG(SDNode *Node) const;
   SDValue ExpandLegalINT_TO_FP(SDNode *Node, SDValue &Chain);
   void PromoteLegalINT_TO_FP(SDNode *N, const SDLoc &dl,
                              SmallVectorImpl<SDValue> &Results);
   void PromoteLegalFP_TO_INT(SDNode *N, const SDLoc &dl,
                              SmallVectorImpl<SDValue> &Results);
 
   SDValue ExpandBITREVERSE(SDValue Op, const SDLoc &dl);
   SDValue ExpandBSWAP(SDValue Op, const SDLoc &dl);
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::ExpandFNEG(SDNode *Node) const {
+  // Expand using integer operations. (We used to expand to FSUB, but that's
+  // not correct in cases involving NaNs.)
+  SDLoc DL(Node);
+
+  // Get the sign bit as an integer.
+  FloatSignAsInt SignAsInt;
+  getSignAsIntValue(SignAsInt, DL, Node->getOperand(0));
+  EVT IntVT = SignAsInt.IntValue.getValueType();
+
+  // Flip the sign.
+  SDValue SignMask = DAG.getConstant(SignAsInt.SignMask, DL, IntVT);
+  SDValue SignFlip = DAG.getNode(ISD::XOR, DL, IntVT, SignAsInt.IntValue,
+                                 SignMask);
+
+  // Convert back to float.
+  return modifySignAsInt(SignAsInt, DL, SignFlip);
+}
+
 SDValue SelectionDAGLegalize::ExpandFABS(SDNode *Node) const {
   SDLoc DL(Node);
   SDValue Value = Node->getOperand(0);
 
   // Transform FABS(x) => FCOPYSIGN(x, 0.0) if FCOPYSIGN is legal.
   EVT FloatVT = Value.getValueType();
   if (TLI.isOperationLegalOrCustom(ISD::FCOPYSIGN, FloatVT)) {
     SDValue Zero = DAG.getConstantFP(0.0, DL, FloatVT);
bool SelectionDAGLegalize::ExpandNode(SDNode *Node) {
   case ISD::GET_DYNAMIC_AREA_OFFSET:
     Results.push_back(DAG.getConstant(0, dl, Node->getValueType(0)));
     Results.push_back(Results[0].getValue(0));
     break;
   case ISD::FCOPYSIGN:
     Results.push_back(ExpandFCOPYSIGN(Node));
     break;
   case ISD::FNEG:
-    // Expand Y = FNEG(X) ->  Y = SUB -0.0, X
-    Tmp1 = DAG.getConstantFP(-0.0, dl, Node->getValueType(0));
-    // TODO: If FNEG has fast-math-flags, propagate them to the FSUB.
-    Tmp1 = DAG.getNode(ISD::FSUB, dl, Node->getValueType(0), Tmp1,
-                       Node->getOperand(0));
-    Results.push_back(Tmp1);
+    Results.push_back(ExpandFNEG(Node));
     break;
   case ISD::FABS:
     Results.push_back(ExpandFABS(Node));
     break;
   case ISD::SMIN:
   case ISD::SMAX:
   case ISD::UMIN:
   case ISD::UMAX: {
     // Expand Y = MAX(A, B) -> Y = (A > B) ? A : B
     ISD::CondCode Pred;
if (!TLI.isStrictFPEnabled() && Results.empty() && Node->isStrictFPOpcode()) {
     switch (Node->getOpcode()) {
     default:
       if (TLI.getStrictFPOperationAction(Node->getOpcode(),
                                          Node->getValueType(0))
           == TargetLowering::Legal)
         return true;
       break;
     case ISD::STRICT_FSUB: {
-      if (TLI.getStrictFPOperationAction(Node->getOpcode(),
+      if (TLI.getStrictFPOperationAction(ISD::STRICT_FSUB,
                                          Node->getValueType(0))
           == TargetLowering::Legal)
         return true;
+      if (TLI.getStrictFPOperationAction(ISD::STRICT_FADD,
+                                         Node->getValueType(0))
+          != TargetLowering::Legal)
+        break;
 
       EVT VT = Node->getValueType(0);
       const SDNodeFlags Flags = Node->getFlags();
       SDValue Neg = DAG.getNode(ISD::FNEG, dl, VT, Node->getOperand(2), Flags);
       SDValue Fadd = DAG.getNode(ISD::STRICT_FADD, dl, Node->getVTList(),
                                  {Node->getOperand(0), Node->getOperand(1), Neg},
                          Flags);
 
       Results.push_back(Fadd);
       Results.push_back(Fadd.getValue(1));
       break;

llvm/test/CodeGen/AArch64/arm64-fp128.ll

 ; CHECK: bl __extenddftf2
 ; CHECK: str q0, [{{x[0-9]+}}, :lo12:lhs]
 
   ret void
 ; CHECK: ret
 }
 
 define fp128 @test_neg(fp128 %in) {
-; CHECK: [[$MINUS0:.LCPI[0-9]+_0]]:
-; Make sure the weird hex constant below *is* -0.0
-; CHECK-NEXT: fp128 -0
-
 ; CHECK-LABEL: test_neg:
 
-  ; Could in principle be optimized to fneg which we can't select, this makes
-  ; sure that doesn't happen.
+;; We convert this to fneg, and target-independent code expands it with
+;; integer operations.
   %ret = fsub fp128 0xL00000000000000008000000000000000, %in
-; CHECK: mov v1.16b, v0.16b
-; CHECK: ldr q0, [{{x[0-9]+}}, :lo12:[[$MINUS0]]]
-; CHECK: bl __subtf3
-
   ret fp128 %ret
-; CHECK: ret
+
+; CHECK:      str q0, [sp, #-16]!
+; CHECK-NEXT: ldrb w8, [sp, #15]
+; CHECK-NEXT: eor w8, w8, #0x80
+; CHECK-NEXT: strb w8, [sp, #15]
+; CHECK-NEXT: ldr q0, [sp], #16
+; CHECK-NEXT: ret
 }

llvm/test/CodeGen/Thumb2/mve-fp-negabs.ll

 entry:
   %0 = fsub nnan ninf nsz <4 x float> <float 0.0e0, float 0.0e0, float 0.0e0, float 0.0e0>, %src
   ret <4 x float> %0
 }
 
 define arm_aapcs_vfpcc <2 x double> @fneg_float64_t(<2 x double> %src) {
 ; CHECK-LABEL: fneg_float64_t:
 ; CHECK:       @ %bb.0: @ %entry
-; CHECK-NEXT:    .save {r4, r5, r7, lr}
-; CHECK-NEXT:    push {r4, r5, r7, lr}
-; CHECK-NEXT:    .vsave {d8, d9}
-; CHECK-NEXT:    vpush {d8, d9}
-; CHECK-NEXT:    vmov q4, q0
-; CHECK-NEXT:    vldr d0, .LCPI2_0
-; CHECK-NEXT:    vmov r2, r3, d9
-; CHECK-NEXT:    vmov r4, r5, d0
-; CHECK-NEXT:    mov r0, r4
-; CHECK-NEXT:    mov r1, r5
-; CHECK-NEXT:    bl __aeabi_dsub
-; CHECK-NEXT:    vmov r2, r3, d8
-; CHECK-NEXT:    vmov d9, r0, r1
-; CHECK-NEXT:    mov r0, r4
-; CHECK-NEXT:    mov r1, r5
-; CHECK-NEXT:    bl __aeabi_dsub
-; CHECK-NEXT:    vmov d8, r0, r1
-; CHECK-NEXT:    vmov q0, q4
-; CHECK-NEXT:    vpop {d8, d9}
-; CHECK-NEXT:    pop {r4, r5, r7, pc}
-; CHECK-NEXT:    .p2align 3
-; CHECK-NEXT:  @ %bb.1:
-; CHECK-NEXT:  .LCPI2_0:
-; CHECK-NEXT:    .long 0 @ double -0
-; CHECK-NEXT:    .long 2147483648
+; CHECK-NEXT:    .pad #16
+; CHECK-NEXT:    sub sp, #16
+; CHECK-NEXT:    vstr d1, [sp]
+; CHECK-NEXT:    ldrb.w r0, [sp, #7]
+; CHECK-NEXT:    vstr d0, [sp, #8]
+; CHECK-NEXT:    ldrb.w r1, [sp, #15]
+; CHECK-NEXT:    eor r0, r0, #128
+; CHECK-NEXT:    strb.w r0, [sp, #7]
+; CHECK-NEXT:    vldr d1, [sp]
+; CHECK-NEXT:    eor r0, r1, #128
+; CHECK-NEXT:    strb.w r0, [sp, #15]
+; CHECK-NEXT:    vldr d0, [sp, #8]
+; CHECK-NEXT:    add sp, #16
+; CHECK-NEXT:    bx lr
 entry:
   %0 = fsub nnan ninf nsz <2 x double> <double 0.0e0, double 0.0e0>, %src
   ret <2 x double> %0
 }
 
 define arm_aapcs_vfpcc <8 x half> @fabs_float16_t(<8 x half> %src) {
 ; CHECK-MVE-LABEL: fabs_float16_t:
 ; CHECK-MVE:       @ %bb.0: @ %entry