LLVM broke frontends relying on documented NaN behavior for minnum/maxnum

[RalfJung]

#139581 (specifically, 17efa57) introduced a breaking change that affects Rust and possibly other frontends: we were relying on the documented, stable, established property that for minnum/maxnum,

If either operand is a NaN, returns the other non-NaN operand.

Unfortunately, that documentation got changed in #112852, and then in 17efa57 the implementation also got changed so now maxnum(sNaN, x) is simplified to qNaN. As a result of that, the code rustc+LLVM generate now no longer matches what we document in the Rust standard library. (This does not completely fix the implementation though. LLVM is currently implementing a messy mix of old and new semantics, which is being discussed in #138451, Discourse, and some other places. That's not what this issue is about; this issue is about the tangible effects on frontends.)

I consider this a regression introduced by #139581; IMO the problematic commit in that PR should be reverted. Changing the behavior of existing intrinsics used by frontends requires more care. Also, https://llvm.org/docs/LangRef.html#llvm-minimumnum-intrinsic exists for those that want the IEEE754-2019 semantics, so it's not clear to me why the old minnum intrinsic has to change. IIUC, 17efa57 has not made it into any release yet, so there's still a chance of fixing this before it affects Rust users.

Cc @LewisCrawford @nikic @arsenm

[RalfJung]

@phoebewang looks like reverting the docs is no longer sufficient. :/

It is worth mentioning that having maxnum(sNaN, x) return x is still permitted by the current docs, because those say that in general,

Floating-point math operations are allowed to treat all NaNs as if they were quiet NaNs. For example, “pow(1.0, SNaN)” may be simplified to 1.0.

So, even without 17efa57, InstSimplify is correct wrt the current docs.

[phoebewang]

@phoebewang looks like reverting the docs is no longer sufficient. :/

It is worth mentioning that having maxnum(sNaN, x) return x is still permitted by the current docs, because those say that in general,

Floating-point math operations are allowed to treat all NaNs as if they were quiet NaNs. For example, “pow(1.0, SNaN)” may be simplified to 1.0.

So, even without 17efa57, InstSimplify is correct wrt the current docs.

I agree with the point, but I think it's already covered by the sentence "return value is non-deterministic if one or both inputs are sNaN" mentioned in #168838. For InstSimplify, simplifying maxnum(sNaN, x) to qNaN doesn't look a problem to me, and conforms to the sentence too. The Behavior of Floating-Point NaN values are general principles for the use of SNaN. The example there is clear enough, we don't need to duplicate it everyone.

[LewisCrawford]

When I implemented #139581 , @arsenm pushed strongly for requiring maxnum(sNaN, x) to produce qNaN instead of relying on the LangRef's wiggle-room allowed by:

Floating-point math operations are allowed to treat all NaNs as if they were quiet NaNs. For example, “pow(1.0, SNaN)” may be simplified to 1.0.

#139581 (comment)

Perhaps a better approach rather than reverting to the old implementation of maxnum(sNaN, x) -> x instead of maxnum(sNaN, x) -> qNaN would be to just avoid optimizing or constant-folding to either version for signalling NaNs, and allow the backends to handle the semantics.

This would require changes to:

• InstSimplify
• DAGCombiner
• ConstantFolding (middle-end, SelectionDAG, and GlobalISel)

[RalfJung]

@phoebewang
Oh interesting, I did not realize that was added there. That's not just a revert of 363b059, that's actually different behavior from the old status quo, right?

So from a Rust perspective, that's just as problematic as the current status quo. We'd like to rely on the guarantee that "If either operand is a NaN, returns the other non-NaN operand", which used to be documented. This means maxnum(sNaN, x) where x is a number (not NaN) must return x. It would be a major hassle for us if LLVM stopped providing this guarantee. What is the motivation for not guaranteeing this any more?

@LewisCrawford

Perhaps a better approach rather than reverting to the old implementation of maxnum(sNaN, x) -> x instead of maxnum(sNaN, x) -> qNaN would be to just avoid optimizing or constant-folding to either version for signalling NaNs, and allow the backends to handle the semantics.

If backends then choose to return a NaN in that situation, that would still break the guarantee which was present for years before #112852. Or was this guarantee historically not actually realized by backends?

[phoebewang]

@phoebewang Oh interesting, I did not realize that was added there. That's not just a revert of 363b059, that's actually different behavior from the old status quo, right?

Yes.

So from a Rust perspective, that's just as problematic as the current status quo. We'd like to rely on the guarantee that "If either operand is a NaN, returns the other non-NaN operand", which used to be documented. This means maxnum(sNaN, x) where x is a number (not NaN) must return x. It would be a major hassle for us if LLVM stopped providing this guarantee. What is the motivation for not guaranteeing this any more?

The motivation was to seek a consensus that every target would agree on. Considering #112852 mentioned aarch64 returns qNaN for sNaN vs NUM. I don't have knowledge on Rust, so this constraint was not in my mind :)

@LewisCrawford

Perhaps a better approach rather than reverting to the old implementation of maxnum(sNaN, x) -> x instead of maxnum(sNaN, x) -> qNaN would be to just avoid optimizing or constant-folding to either version for signalling NaNs, and allow the backends to handle the semantics.

If backends then choose to return a NaN in that situation, that would still break the guarantee which was present for years before #112852. Or was this guarantee historically not actually realized by backends?

I agree disabling optimizations is not the right direction. The reason is the same as I objecting extra code generation in the backend to handle it. We should not give up performance unnecessarily.

[phoebewang]

So from a Rust perspective, that's just as problematic as the current status quo. We'd like to rely on the guarantee that "If either operand is a NaN, returns the other non-NaN operand", which used to be documented. This means maxnum(sNaN, x) where x is a number (not NaN) must return x. It would be a major hassle for us if LLVM stopped providing this guarantee.

I saw @valadaptive 's [RFC] A consistent set of semantics for the floating-point minimum and maximum operations. I think as a compromise, we may keep the old behavior for llvm.minnum.* and llvm.maxnum.* for Rust and similar users, while switching the C representation to llvm.minimumnum.* and llvm.maximumnum.* with the nsz.

[RalfJung]

aarch64 returns qNaN for sNaN vs NUM

Is that the lowering LLVM already uses (or used even before #112852) for minnum/maxnum? Or is the idea to eventually allow a slightly more efficient lowering on aarch64 by avoiding the need to first silence NaNs?

[valadaptive]

Or is the idea to eventually allow a slightly more efficient lowering on aarch64 by avoiding the need to first silence NaNs?

I was concerned about performance as well, but LLVM seems quite capable of eliding the extra "canonicalize" optimizations (on aarch64, "canonicalize" is lowered to "take the minimum of a number with itself"). Since basically every floating-point math operation silences NaNs, this should be really easy to statically determine.