OpenCL-CTS icon indicating copy to clipboard operation
OpenCL-CTS copied to clipboard

math_brute_force: fmin/fmax can return a qNaN if either input is a sNaN

Open changpeng opened this issue 9 months ago • 8 comments

For fmin/fmax, when either argument is a signaling NaN, a quiet NaN return is also acceptable, which is respect to C99, where signaling NaNs are supposed to get the same IEEE treatment.

changpeng avatar Feb 20 '25 01:02 changpeng

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 20 '25 01:02 CLAassistant

From my perspective, the C spec does not distinguish between QNaN and SNaN in its description of fmax() and fmin(), so this PR, if it is accepted, might need a spec update as well.

b-sumner avatar Feb 20 '25 02:02 b-sumner

I haven't reviewed this CTS PR, but the description sounds consistent with the current spec, so I think we're OK there. Specifically, refer to the discussion on https://github.com/KhronosGroup/OpenCL-Docs/pull/128 - this is an old PR, but I don't think anything has changed in the spec in this area.

Also, note this footnote in the C spec, which applies to fmin and fmax specifically: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#_footnotedef_43

bashbaug avatar Feb 20 '25 04:02 bashbaug

Personally, I don't think the footnote is good enough. The spec for fmax/fmin says only NaN, and that means "any kind of NaN". If we allow fmax/fmin to return a QNaN when one of the arguments is not a NaN then we are disobeying the current spec.

b-sumner avatar Feb 20 '25 14:02 b-sumner

Sorry, I'm not following (my fault). Can you describe the concern in more detail?

AFAICT, this PR is specifically about whether fmin and fmax must return a sNaN when an input is a sNaN, or whether fmin and fmax may return a qNaN when either (or both) inputs are sNaN. I think the spec is pretty clear about this, both in Section 7.2 which says "Support for signaling NaNs is not required", and then in Footnote 43, which specifically cautions that for fmin and fmax "signaling NaNs may behave as quiet NaNs".

Note, if this is going to need a longer discussion we should spin it off into a separate OpenCL-Docs issue.

bashbaug avatar Feb 20 '25 16:02 bashbaug

The concern here is that the patch wants fmax/fmin to be able to return a QNaN if one or more of the arguments is SNaN. This is specifically not permitted by the spec for these functions which says that if one argument is (any kind of) NaN, and the other is not, the other argument is returned.

b-sumner avatar Feb 20 '25 17:02 b-sumner

AFAICT, this PR is specifically about whether fmin and fmax must return a sNaN when an input is a sNaN

Not exactly, the result should be a quiet nan. The IEEE-754 2008 behavior for minNum and maxNum is to return a quiet nan if either input is a signaling nan. This is backwards from the treatment of a quiet nan input, in which case the numeric result argument is returned.

The current conformance test behavior is checking for the result of fmin matches minNum(quiet(src0), quiet(src1)), thus avoiding the inversion of behavior. This was the buggy glibc implementation of fmin/fmax for years, but this was fixed (https://sourceware.org/bugzilla/show_bug.cgi?id=20947)

Is it possible something else is going on here? Note, we just merged a fix related to fmin and fmax and NaNs: #2270

Yes. The confusion is the IEEE behavior for signaling nans is inverted. It is not a matter of whether the result is a quiet or signaling nan, but whether the result is a nan or non-nan for a signaling nan input.

https://github.com/KhronosGroup/OpenCL-Docs/pull/128

This PR does not discuss fmin and fmax, which are a special case.

Is it possible something else is going on here? Note, we just merged a fix related to fmin and fmax and NaNs: https://github.com/KhronosGroup/OpenCL-CTS/pull/2270

This will not address this issue. This is still treating the signalingness as insignificant.

which specifically cautions that for fmin and fmax "signaling NaNs may behave as quiet NaNs".

What the conformance test is currently checking is that signaling nan inputs must behave as quiet nans. This change is intended to relax this to allow signaling nans to behave as signaling nans

arsenm avatar Feb 21 '25 02:02 arsenm

I don't think we can change bruteforce without changing the C spec which specifically does not allow returning a NaN unless both arguments are NaN.

b-sumner avatar Mar 31 '25 17:03 b-sumner