KhronosGroup / GLSL

GLSL Shading Language Specification and Extensions
Other
335 stars 97 forks source link

significand range for frexp() incorrect due to sign-handling #71

Open nothings opened 5 years ago

nothings commented 5 years ago

The specification says the range of the significand (aka mantissa) returned by frexp() is [0.5,1.0]. Handling of the sign of the floating point number is not mentioned except for the special case of minus zero.

In C/C++ frexp() preserves the sign in the significand, so the range is documented as [-1.0,-0.5) and (0.5,1.0]. NVIDIA frexp() appears to preserve the sign in the significand. Given minus zero preservation, I believe sign-preservation was the intent and the specification is in error.

Related, the C/C++ ranges are documented as exclusive/inclusive to guarantee you won't get 0.5 as a significand. The inclusive-inclusive range in the GLSL specification deviates from this, presumably intentionally to allow flexibility in the implementation. (For example, it appears that the NVIDIA GPU on this desktop returns 0.5 for frexp(1.0,exp).) In my opinion it would be nice to be explicit about this deviation from C/C++ instead of expecting people to infer an unusual behavior from a single unemphasized character being '[' instead of '('.

Also related, ARB_gpu_shader5 documents the range as [0.5,1.0), with the inclusive-exclusive reversed from C/C++. Again presumably intentionally, but I imagine it should be made consistent with the GLSL specification if it gets the sign fix.

To summarize, the following are the significand ranges found for frexp() in various places:

[0.5,1.0] - GLSL 4.60 specification (and earlier) [0.5,1.0) - ARB_gpu_shader5 version 16 (2012) (0.5,1.0] & [-1.0,-0.5) - C/C++ [0.5,1.0] & [-1.0,-0.5] - proposed corrected GLSL range

(Note that I have reversed the order of the pairs of ranges above from their natural ordering to make visual comparison simpler.)

The handling of sign MUST be corrected, and the inconsistency with C/C++ SHOULD be documented explicitly.

krolli commented 5 years ago

Hello,

I just did a little bit of searching and, I may be wrong, but I think C/C++ frexp() is defined to return ranges (-1;-0.5] & [0.5; 1). Here are links I based this on. http://www.cplusplus.com/reference/cmath/frexp/ https://en.cppreference.com/w/c/numeric/math/frexp http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf (7.12.6.4)

Another thing I noticed in https://www.khronos.org/registry/OpenGL/specs/gl/GLSLangSpec.4.60.html is that GLSL specification seems to be worded a little weird, when compared to C specification working draft above. First it says "Splits x into a floating-point significand in the range [0.5,1.0]" and then it requires that "x = significant · 2^exponent" which, to me, sounds like it wouldn't work for negative values. C specification talks about "magnitude" which I take as "absolute value" and that would make more sense.

johnkslang commented 5 years ago

Yes, right. I realize now there has been discussion internal to Khronos that agrees with what you're saying, but it is just now getting agreed on, with the results to be published soon.

pdaniell-nv commented 5 years ago

It appears the existing dEQP-GLES31.functional.shaders.builtin_functions.common.frexp.* tests cover the case of negative operands and expect negative results. So all passing implementations should behave correctly.

dj2 commented 4 weeks ago

Is the agreed behaviour to update the spec to have the range of [0.5,1.0] & [-1.0,-0.5] for GLSL?