[InstCombine] preserve signbit semantics of NAN with fold to fabs

[spatel]

Summary

As discussed in issue #59279, we want fneg/fabs to conform to the IEEE-754 spec for signbit operations - quoting from section 5.5.1 of IEEE-754-2008:
"negate(x) copies a floating-point operand x to a destination in the same format, reversing the sign bit"
"abs(x) copies a floating-point operand x to a destination in the same format, setting the sign bit to 0 (positive)"
"The operations treat floating-point numbers and NaNs alike."

So we gate this transform with "nnan" in addition to "nsz":
(X > 0.0) ? X : -X --> fabs(X)

Without that restriction, we could have for example:
(+NaN > 0.0) ? +NaN : -NaN --> -NaN (because an ordered compare with NaN is always false)
That would be different than fabs(+NaN) --> +NaN.

More fabs/fneg patterns demonstrated here:
https://godbolt.org/z/h8ecc659d
(without any FMF, these are correct independently of this patch - no fabs should be created)

The code change is a one-liner, but we have lots of tests diffs because there are many variations of the basic pattern.

[RalfJung]

I am a bit confused by the explanation. The signbit of NaN is "insignificant" for *some* operations like + in the sense that they just pick an arbitrary sign bit if the result is NaN (and so the sign bit of a NaN input has no effect). But it affects the behavior of >, fneg, and fabs (the operations involved here) in totally deterministic ways, right? When only >, fneg, fabs are used (and copies of the floating point value from one register to another), the sign bit is fully significant, like all the others.

The actual issue is that X > 0.0 is false for negative NaNs, so the LHS returns X in that case -- meaning the LHS can sometimes return a value with the sign bit set, but the RHS will never have the sign bit set. Hence the transformation sometimes produces a wrong sign bit. And that bit is significant enough to make this an incorrect transformation.

I think it is hence very confusing to claim that the sign bit of a NaN is insignificant/meaningless in general.

Or did I misunderstand something?

[nlopes]

In D139785#3986910, @RalfJung wrote:

The actual issue is that X > 0.0 is false for negative NaNs

Comparisons don't look at the sign of NaN. They are not bitwise operations like fabs/fneg.

[RalfJung]

Comparisons don't look at the sign of NaN. They are not bitwise operations like fabs/fneg.

Yeah, and hence -NaN > 0.0 is false, so that's what I said, no?

My confusion is with this comment

// Note: This requires nnan to preserve signbit semantics even though the
//       signbit of a NAN is insignificant.

since "signbit of a NAN is insignificant" is not correct in general.

I would be less confused by something like

// Note: This requires nnan to preserve signbit semantics since fcmp ignores
//       the signbit of a NAN.

[spatel]

In D139785#3986980, @RalfJung wrote:

Comparisons don't look at the sign of NaN. They are not bitwise operations like fabs/fneg.

Yeah, and hence -NaN > 0.0 is false, so that's what I said, no?

My confusion is with this comment

// Note: This requires nnan to preserve signbit semantics even though the
//       signbit of a NAN is insignificant.

since "signbit of a NAN is insignificant" is not correct in general.

I would be less confused by something like

// Note: This requires nnan to preserve signbit semantics since fcmp ignores
//       the signbit of a NAN.

The confusion comes from the IEEE spec itself - the signbit of NAN is never meaningful in real code, but the spec requires that the neg/abs/copysign operations treat the signbit of NAN deterministically anyway. This is what we went back and forth on in the bug report. Ie, we could implement the less strict interpretation, and it should not affect functionality of any real program. The code/test comments were intending to suggest that alternative interpretation if we ever decided that this way was overkill. I'm ok changing the comment if that's preferred.

[RalfJung]

the signbit of NAN is never meaningful in real code,

This is the statement I am still objecting to. It *is* meaningful in the sense that (a) code can look at it if it wants, and (b) some operations guarantee to treat it in a particular way (in particular, float Y = true ? X1 : X2; guarantees that Y has the same signbit as X1).

The signbit is never meaningful in code that only treats float values as approximation to mathematical real numbers, but LLVM must work on other code, too. For that reason I think the mindset of "it is not meaningful but ..." is confusing, and easily leads to misunderstandings and bugs such as the one this change is fixing.

[spatel]

In D139785#3987002, @RalfJung wrote:

The signbit is never meaningful in code that only treats float values as approximation to mathematical real numbers, but LLVM must work on other code, too. For that reason I think the mindset of "it is not meaningful but ..." is confusing, and easily leads to misunderstandings and bugs such as the one this change is fixing.

I'm not convinced that this is really a bug, but I'm ok changing the wording. :)

By default, LLVM IR implements a subset of IEEE FP to allow optimizations that IEEE does not:
https://llvm.org/docs/LangRef.html#floating-point-environment
(also note that we support targets that are even less IEEE-compliant than the IR spec, so we don't do some transforms that would be allowed if we could assume an IEEE-compliant target)

So it's up to us to decide if this quirk of NAN and signbit-op behavior is worth implementing. This will make Alive2 more useful, and I'm hoping it won't harm optimization in real code. If it does harm optimization, we could switch back (leave this transform as-is). In other words, we would make it explicit that the signbit of NAN is meaningless as it was (briefly) implemented in Alive2. But then we would lose some other optimization as noted in the bug report.

[spatel]

Patch updated:
Adjusted code comment - we are implementing a model where the signbit is specified by fabs/fneg, so don't say it is meaningless.

[RalfJung]

Note that semantics of fneg/fabs is irrelevant for this optimization to be a problem. That's why I keep using float Y = true ? X1 : X2; as the example. To my knowledge nobody disputes that NaN < 0.0 must deterministically return false, so X < 0.0 ? -X : X will behave the same as false ? -X : X when X is a NAN, which is the same as just X -- I hope this much is uncontroversial. The only potential question then is whether float Y = X must preserve the NAN sign bit of X.

float X = /* NaN with sign bit set */;
float Y1 = X < 0.0 ? -X : X; // just a convoluted way to write `Y1 = false ? ... : X`, aka `Y1 = X`.
float Y2 = X;
// Clearly Y1 and Y2 must be bit-equal here, since both are just copies of X?
// But making Y1 be fabs(X) will destroy that property no matter whether or
// not we specify `fabs` to behave deterministically on NaNs.

I personally would be extremely surprised if just copying a value was allowed to alter any of its bits. Is that the alternative NAN semantics you meant, or am I misunderstanding something?

[spatel]

In D139785#3987049, @RalfJung wrote:

Note that semantics of fneg/fabs is irrelevant for this optimization to be a problem. That's why I keep using float Y = true ? X1 : X2; as the example. To my knowledge nobody disputes that NaN < 0.0 must deterministically return false, so X < 0.0 ? -X : X will behave the same as false ? -X : X when X is a NAN, which is the same as just X -- I hope this much is uncontroversial. The only potential question then is whether float Y = X must preserve the NAN sign bit of X.

float X = /* NaN with sign bit set */;
float Y1 = X < 0.0 ? -X : X; // just a convoluted way to write `Y1 = false ? ... : X`, aka `Y1 = X`.
float Y2 = X;
// Clearly Y1 and Y2 must be bit-equal here, since both are just copies of X?

I think there's still some ambiguity in this example. How was X initialized/observed with -NaN? Testing for bit-equality requires casting to integer. There's no way to do that comparison with FP values?

Note that we also don't guarantee that NaN payload bits propagate. That part is less well-specified in IEEE-754, so maybe it's less controversial?

I personally would be extremely surprised if just copying a value was allowed to alter any of its bits. Is that the alternative NAN semantics you meant, or am I misunderstanding something?

Yes, I think that's right. But as I suggested above, this is already the case and continues independently of this patch. NaN payload bits are part of this problem/discussion too, right?
https://discourse.llvm.org/t/semantics-of-nan/66729/11

[RalfJung]

How was X initialized/observed with -NaN? Testing for bit-equality requires casting to integer. There's no way to do that comparison with FP values?

Yes, this assumes we can convert between float and i32 freely, and non-poison/undef values are preserved perfectly with all their bits when doing that. If this is not true, the LangRef surely must contain big fat warnings about that, and I doubt Rust would be the only frontend that would be broken badly by such a decision.

Note that we also don't guarantee that NaN payload bits propagate. That part is less well-specified in IEEE-754, so maybe it's less controversial?

They do propagate perfectly in copy, copysign, fneg, fabs according to IEEE, is my understanding.

[spatel]

Ping.

This patch gets us closer to the desired behavior of IEEE-compliant fneg/fabs/copysign (there is no "copy" in IR) semantics with the default LLVM FP environment, and the code comment is hopefully clear now.

If there are other open questions, those should be added to the discourse thread or in bug reports.

[lebedev.ri]

Could you add a relevant quote of the IEE spec into the description,
and add an example with +NaN and -NaN and describe the outcome (kinda like alive2 counter-example)?

[spatel]

In D139785#4016955, @lebedev.ri wrote:

Could you add a relevant quote of the IEE spec into the description,
and add an example with +NaN and -NaN and describe the outcome (kinda like alive2 counter-example)?

Sure - see updated text. Let me know if you'd like to see something else or different.

[lebedev.ri]

In D139785#4016957, @spatel wrote:

In D139785#4016955, @lebedev.ri wrote:

Could you add a relevant quote of the IEE spec into the description,
and add an example with +NaN and -NaN and describe the outcome (kinda like alive2 counter-example)?

Sure - see updated text. Let me know if you'd like to see something else or different.

Consider also adding https://godbolt.org/z/h9aefnKhE

[lebedev.ri]

In D139785#4016958, @lebedev.ri wrote:

In D139785#4016957, @spatel wrote:

In D139785#4016955, @lebedev.ri wrote:

Could you add a relevant quote of the IEE spec into the description,
and add an example with +NaN and -NaN and describe the outcome (kinda like alive2 counter-example)?

Sure - see updated text. Let me know if you'd like to see something else or different.

Consider also adding https://godbolt.org/z/h9aefnKhE

Err, more like https://godbolt.org/z/1h161jEEj (or even https://godbolt.org/z/fe3sKa9Yc)

[spatel]

In D139785#4016959, @lebedev.ri wrote:

Err, more like https://godbolt.org/z/1h161jEEj (or even https://godbolt.org/z/fe3sKa9Yc)

Thanks! Added a similar link and explanation to the patch summary.

[lebedev.ri]

I'm not very good with FP, but this does seem well-justified.
Thanks.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

for (bool Swap : {false, true}) {
     }
 
     // With nsz, when 'Swap' is false:
     // fold (X < +/-0.0) ? -X : X or (X <= +/-0.0) ? -X : X to fabs(X)
     // fold (X > +/-0.0) ? -X : X or (X >= +/-0.0) ? -X : X to -fabs(x)
     // when 'Swap' is true:
     // fold (X > +/-0.0) ? X : -X or (X >= +/-0.0) ? X : -X to fabs(X)
     // fold (X < +/-0.0) ? X : -X or (X <= +/-0.0) ? X : -X to -fabs(X)
-    if (!SI.hasNoSignedZeros())
+    //
+    // Note: We require "nnan" for this fold because fcmp ignores the signbit
+    //       of NAN, but IEEE-754 specifies the signbit of NAN values with
+    //       fneg/fabs operations.
+    if (!SI.hasNoSignedZeros() || !SI.hasNoNaNs())
       return nullptr;
 
     if (Swap)
       Pred = FCmpInst::getSwappedPredicate(Pred);
 
     bool IsLTOrLE = Pred == FCmpInst::FCMP_OLT || Pred == FCmpInst::FCMP_OLE ||
                     Pred == FCmpInst::FCMP_ULT || Pred == FCmpInst::FCMP_ULE;
     bool IsGTOrGE = Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE ||

llvm/test/Transforms/InstCombine/fabs.ll

 ; CHECK-NEXT:    ret fp128 [[TMP1]]
 ;
   %gtzero = fcmp ogt fp128 %x, zeroinitializer
   %negx = fsub fp128 zeroinitializer, %x
   %fabs = select i1 %gtzero, fp128 %x, fp128 %negx
   ret fp128 %fabs
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define float @select_nsz_fcmp_ogt_fneg(float %a) {
 ; CHECK-LABEL: @select_nsz_fcmp_ogt_fneg(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.fabs.f32(float [[A:%.*]])
-; CHECK-NEXT:    ret float [[TMP1]]
+; CHECK-NEXT:    [[FNEG:%.*]] = fneg float [[A:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ogt float [[A]], 0.000000e+00
+; CHECK-NEXT:    [[R:%.*]] = select nsz i1 [[CMP]], float [[A]], float [[FNEG]]
+; CHECK-NEXT:    ret float [[R]]
 ;
   %fneg = fneg float %a
   %cmp = fcmp ogt float %a, %fneg
   %r = select nsz i1 %cmp, float %a, float %fneg
   ret float %r
 }
 
 define float @select_nsz_nnan_fcmp_ogt_fneg(float %a) {

 ; CHECK-NEXT:    ret half [[FABS]]
 ;
   %gtzero = fcmp oge half %x, -0.0
   %negx = fsub nnan half 0.0, %x
   %fabs = select i1 %gtzero, half %x, half %negx
   ret half %fabs
 }
 
-; X < 0.0 ? -X : X --> fabs(X)
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
 
 define double @select_nsz_fcmp_olt_zero_unary_fneg(double %x) {
 ; CHECK-LABEL: @select_nsz_fcmp_olt_zero_unary_fneg(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    ret double [[TMP1]]
+; CHECK-NEXT:    [[LTZERO:%.*]] = fcmp olt double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[FABS:%.*]] = select nsz i1 [[LTZERO]], double [[NEGX]], double [[X]]
+; CHECK-NEXT:    ret double [[FABS]]
 ;
   %ltzero = fcmp olt double %x, 0.0
   %negx = fneg double %x
   %fabs = select nsz i1 %ltzero, double %negx, double %x
   ret double %fabs
 }
 
+; X < 0.0 ? -X : X --> fabs(X)
+
 define double @select_nsz_nnan_fcmp_olt_zero_unary_fneg(double %x) {
 ; CHECK-LABEL: @select_nsz_nnan_fcmp_olt_zero_unary_fneg(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %ltzero = fcmp olt double %x, 0.0
   %negx = fneg double %x
   %fabs = select nsz nnan i1 %ltzero, double %negx, double %x

 ; CHECK-NEXT:    ret float [[FABS]]
 ;
   %lezero = fcmp ule float %x, -0.0
   %negx = fneg nnan nsz float %x
   %fabs = select i1 %lezero, float %negx, float %x
   ret float %fabs
 }
 
-; X > 0.0 ? X : (-X) --> fabs(X)
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
 
 define <2 x float> @select_nsz_fcmp_ogt_zero_unary_fneg(<2 x float> %x) {
 ; CHECK-LABEL: @select_nsz_fcmp_ogt_zero_unary_fneg(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz <2 x float> @llvm.fabs.v2f32(<2 x float> [[X:%.*]])
-; CHECK-NEXT:    ret <2 x float> [[TMP1]]
+; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp ogt <2 x float> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg <2 x float> [[X]]
+; CHECK-NEXT:    [[FABS:%.*]] = select nsz <2 x i1> [[GTZERO]], <2 x float> [[X]], <2 x float> [[NEGX]]
+; CHECK-NEXT:    ret <2 x float> [[FABS]]
 ;
   %gtzero = fcmp ogt <2 x float> %x, zeroinitializer
   %negx = fneg <2 x float> %x
   %fabs = select nsz <2 x i1> %gtzero, <2 x float> %x, <2 x float> %negx
   ret <2 x float> %fabs
 }
 
+; X > 0.0 ? X : (-X) --> fabs(X)
+
 define <2 x float> @select_nsz_nnan_fcmp_ogt_zero_unary_fneg(<2 x float> %x) {
 ; CHECK-LABEL: @select_nsz_nnan_fcmp_ogt_zero_unary_fneg(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz <2 x float> @llvm.fabs.v2f32(<2 x float> [[X:%.*]])
 ; CHECK-NEXT:    ret <2 x float> [[TMP1]]
 ;
   %gtzero = fcmp ogt <2 x float> %x, zeroinitializer
   %negx = fneg <2 x float> %x
   %fabs = select nsz nnan <2 x i1> %gtzero, <2 x float> %x, <2 x float> %negx

llvm/test/Transforms/InstCombine/fneg-fabs.ll

;
   %cmp = fcmp olt double %x, 0.000000e+00
   %negX = fneg fast double %x
   %sel = select nsz nnan i1 %cmp, double %x, double %negX
   ret double %sel
 }
 
 ; Tests with various predicate types.
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_ult(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_ult(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ult double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[X]], double [[NEGX]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ult double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %x, double %negX
   ret double %sel
 }
 
 define double @select_nsz_nnan_nfabs_ult(double %x) {
 ; CHECK-LABEL: @select_nsz_nnan_nfabs_ult(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    [[SEL:%.*]] = fneg nnan nsz double [[TMP1]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ult double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz nnan i1 %cmp, double %x, double %negX
   ret double %sel
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_ole(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_ole(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ole double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[X]], double [[NEGX]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ole double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %x, double %negX
   ret double %sel
 }
 
 define double @select_nsz_nnan_nfabs_ole(double %x) {
 ; CHECK-LABEL: @select_nsz_nnan_nfabs_ole(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    [[SEL:%.*]] = fneg nnan nsz double [[TMP1]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ole double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz nnan i1 %cmp, double %x, double %negX
   ret double %sel
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_ule(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_ule(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ule double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[X]], double [[NEGX]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ule double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %x, double %negX
   ret double %sel
 }
 

 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ogt double %x, 0.000000e+00
   %negX = fneg fast double %x
   %sel = select nsz nnan i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
-; Tests with various predicate types.
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_ogt(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_ogt(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ogt double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[NEGX]], double [[X]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ogt double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
+; Tests with various predicate types.
+
 define double @select_nsz_nnan_nfabs_ogt(double %x) {
 ; CHECK-LABEL: @select_nsz_nnan_nfabs_ogt(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    [[SEL:%.*]] = fneg nnan nsz double [[TMP1]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ogt double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz nnan i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_ugt(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_ugt(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ugt double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[NEGX]], double [[X]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ugt double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
 define double @select_nsz_nnan_nfabs_ugt(double %x) {
 ; CHECK-LABEL: @select_nsz_nnan_nfabs_ugt(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    [[SEL:%.*]] = fneg nnan nsz double [[TMP1]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp ugt double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz nnan i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_oge(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_oge(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp oge double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[NEGX]], double [[X]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp oge double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
 define double @select_nsz_nnan_nfabs_oge(double %x) {
 ; CHECK-LABEL: @select_nsz_nnan_nfabs_oge(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    [[SEL:%.*]] = fneg nnan nsz double [[TMP1]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp oge double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz nnan i1 %cmp, double %negX, double %x
   ret double %sel
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define double @select_nsz_nfabs_uge(double %x) {
 ; CHECK-LABEL: @select_nsz_nfabs_uge(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[SEL:%.*]] = fneg nsz double [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp uge double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[NEGX:%.*]] = fneg double [[X]]
+; CHECK-NEXT:    [[SEL:%.*]] = select nsz i1 [[CMP]], double [[NEGX]], double [[X]]
 ; CHECK-NEXT:    ret double [[SEL]]
 ;
   %cmp = fcmp uge double %x, 0.000000e+00
   %negX = fneg double %x
   %sel = select nsz i1 %cmp, double %negX, double %x
   ret double %sel
 }
 

llvm/test/Transforms/InstCombine/fneg.ll

 ;
   %fneg = fneg float %a
   %cmp = fcmp ogt float %a, %fneg
   %sel = select i1 %cmp, float %a, float %fneg
   %fneg1 = fneg fast float %sel
   ret float %fneg1
 }
 
+; This is not fabs because that could produce a different signbit for a NAN input.
+; PR59279
+
 define float @fnabs_2_nsz(float %a) {
 ; CHECK-LABEL: @fnabs_2_nsz(
-; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.fabs.f32(float [[A:%.*]])
-; CHECK-NEXT:    [[FNEG1:%.*]] = fneg float [[TMP1]]
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp olt float [[A:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[A_NEG:%.*]] = fneg float [[A]]
+; CHECK-NEXT:    [[FNEG1:%.*]] = select nsz i1 [[CMP]], float [[A]], float [[A_NEG]]
 ; CHECK-NEXT:    ret float [[FNEG1]]
 ;
   %fneg = fneg float %a
   %cmp = fcmp olt float %a, %fneg
   %sel = select nsz i1 %cmp, float %fneg, float %a
   %fneg1 = fneg float %sel
   ret float %fneg1
 }