Skip to content

Handle sNaNs in {min,max}_precise on Neon - #441

Open
RunDevelopment wants to merge 3 commits into
linebender:mainfrom
RunDevelopment:min-max-precise
Open

RunDevelopment wants to merge 3 commits into
linebender:mainfrom
RunDevelopment:min-max-precise

Conversation

@RunDevelopment

Copy link
Copy Markdown
Contributor

Motivation

The _precise family of methods is supposed to return platform-independent precise results at the cost of speed. So it was very surprising to me when I read that max_precise didn't handle NaNs correctly. To quote the docs:

If a floating-point operand is a signaling NaN, the result is not just implementation-defined, but fully non-deterministic: it may be either NaN or the non-NaN operand. Signaling NaN values are not produced by floating-point math operations, only from manual initialization with specific bit patterns. You probably don’t need to worry about them.

In my case, the numbers I get are user input (they could be anything) so I can't just assume sNaNs aren't included. Plus, no other operation has this caveat around sNaNs and neither does the standard library with fN::max.

Changes

This PR removes the caveat around sNaN by bringing Neon in line. All other platforms already handled NaNs (signally or not) like the standard library.

To bring Neon in line, I'm using the same trick as the standard library. vmaxnmq_f32 is the element-wise max operation, and it also has the property of never returning sNaNs. So vmaxnmq_f32(x, x) will return a quiet NaN if x is any NaN value (signalling or not) and the numeric value of x otherwise. With this trick, sNaN disappears.

Example: max_precise(a, b) is implemented like this:

vmaxnmq_f32(
    vmaxnmq_f32(a, a),
    vmaxnmq_f32(b, b)
)

For consistency, I also adjusted reduce_{max,min}_precise to handle sNaN correctly using the same trick. However, the standard reduction from larger-than-native vectors would have resulted in a few unnecessary sNaN -> qNaN conversions. To avoid them, I also implemented a custom reduce logic to skip the unnecessary conversions.

@Shnatsel

Shnatsel commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

There is a much more significant problem with these methods:

If one floating-point operand is positive zero and the other is negative zero, the result is implementation-defined, and it could be either one.

It's a much bigger issue for determinism in practice than the theoretical possibility of signalling NaNs.

If you want fully IEEE compliant min/max that deals with all kinds of NaN and also signed zero, you need a much slower formulation. People have workshopped variants of it in this thread: https://news.ycombinator.com/item?id=49800085

I don't want to slow down the _precise variant for everyone because of signalling NaNs, but I am open to adding a min_ieee or some such that is fully deterministic at the cost of speed.

@RunDevelopment

Copy link
Copy Markdown
Contributor Author

My goal was to make them behave the same as the standard library by getting rid of the NaN caveat. Since the standard library doesn't care about non-deterministic +/-0, neither do I.

IMO, implementing IEEE 754's minimum/maximum should also follow the (unstable) standard library. But implementing those is a different issue from bringing the semantics of max_precise (and co.) in line with the standard library.

I don't want to slow down the _precise variant for everyone because of signalling NaNs

max_precise compiles to 3 instructions now. Same as WASM, AVX2, and SSE4.2. The slowdown is within what should be expected for the additional guarantees IMO.

Plus, the compiler can optimize away vmaxnmq_f32(x, x) when it knows x isn't NaN, so code like input.max_precise(0.0).min_precise(1.0) is 3 instructions. That's a lot better than other platforms (except AVX512), where max_precise could be implemented as a single instruction when one arg is known to be non-NaN, but the compiler isn't smart enough for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants