[llvm] [LLVM] Slay undead copysign code (PR #111269)

[via llvm-commits]

================
@@ -1649,12 +1649,19 @@ void DAGTypeLegalizer::ExpandFloatRes_FCEIL(SDNode *N,
 
 void DAGTypeLegalizer::ExpandFloatRes_FCOPYSIGN(SDNode *N,
                                                 SDValue &Lo, SDValue &Hi) {
-  ExpandFloatRes_Binary(N, GetFPLibCall(N->getValueType(0),
-                                        RTLIB::COPYSIGN_F32,
-                                        RTLIB::COPYSIGN_F64,
-                                        RTLIB::COPYSIGN_F80,
-                                        RTLIB::COPYSIGN_F128,
-                                        RTLIB::COPYSIGN_PPCF128), Lo, Hi);
+
+  assert(N->getValueType(0) == MVT::ppcf128 &&
+         "Logic only correct for ppcf128!");
+  SDLoc DL = SDLoc(N);
+  SDValue Tmp = SDValue();
+  GetExpandedFloat(N->getOperand(0), Lo, Tmp);
+
+  Hi = DAG.getNode(ISD::FCOPYSIGN, DL, Tmp.getValueType(), Tmp,
+                   N->getOperand(1));
+  // a double-double is Hi + Lo, so if Hi flips sign, so must Lo
+  Lo = DAG.getSelectCC(DL, Tmp, Hi, Lo,
+                       DAG.getNode(ISD::FNEG, DL, Lo.getValueType(), Lo),
+                       ISD::SETEQ);
----------------
workingjubilee wrote:

I am not aware of a rule that says that the two floats have to both be the same sign, which would be required for a second `copysign` to make sense. The only one that I am aware of that is relevant is that `Hi` has to be greater. That's why it has to be a _flip_ of the _existing_ sign of `Lo`. We are not trying to apply the logic of `copysign` to the individual parts, but to the "summed float".

For a given float it might go something like this (I suspect most real cases would not have these be so close in orders of magnitude but...):
```
dbldbl = { Hi = 27.0, Lo = -4.0 };
assert(dbldbl == 23.0);
dbldbl = copysign(dbldbl, -1.0);
assert(dbldbl.Hi == -27.0);
assert(dbldbl.Lo == 4.0);
assert(dbldbl == -23.0);
```

So no, it cannot be "another copysign", I think, as that would leave us with this possibility:
```
dbldbl = copysign(dbldbl, -1.0);
assert(dbldbl.Hi == -27.0);
assert(dbldbl.Lo == -4.0);
assert(dbldbl == -27.0);
```

This pattern is also used in `ExpandFloatRes_FABS`, here:
https://github.com/llvm/llvm-project/blob/4a0caf627bc67b99f59518e50d482453575588c4/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp#L1557-L1560

We should only ever, when copying the sign _from_ this, take the value from `Hi`, per here, since it is the sign that matters:
https://github.com/llvm/llvm-project/blob/367c3c968eb8f29b55fb8019b2464c7ff6307ca8/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp#L2205-L2208

Thus it should not break NaN float sign handling, as the sign of `Lo` should be of no consequence to a NaN `ppc_fp128`, at least "externally" to the float.

https://github.com/llvm/llvm-project/pull/111269