[PATCH] D83473: libclc: Fix FP_ILOGBNAN definition

Fri Jul 10 20:30:34 PDT 2020

jvesely added a comment.

tldr; I don't mind changing this to be in sync with clang, just get the patch right. 
However, it's not the right way to guarantee IR level compatibility between clang-cl-headers and libclc.
details below

In D83473#2145073 <https://reviews.llvm.org/D83473#2145073>, @jenatali wrote:

> @jvesely, I think libclc needs to change its definition here, as it's the only one out of 3 OpenCL standard lib headers that's different.

There are two separate issues
a) whether to change libclc
b) doing the change in the correct way

> Since OpenCL allows apps to provide precompiled intermediate representations to the API, in the form of SPIR or SPIR-V, that means that the app could have embedded references to `FP_ILOGBNAN`, which are just a constant value since it's a `#define`, in their own code. They could write a kernel which calls `ilogb` and compares the result to `FP_ILOGBNAN`, and compile that with either clang trunk (which uses opencl-c.h automatically), or with Khronos's offline compiler (https://github.com/KhronosGroup/SPIR) using Khronos's standard library headers (https://github.com/KhronosGroup/libclcxx). Both of these standard library implementation headers define `FP_ILOGBNAN` to be `INT_MAX`:
> 
> - https://github.com/llvm/llvm-project/blob/master/clang/lib/Headers/opencl-c-base.h#L165
> - https://github.com/KhronosGroup/libclcxx/blob/96459f111c3e3a4709f7e09bf5fb73dea81a475a/include/opencl_math_constants#L85

I've no problem with the compatibility argument. but using `INT_MAX` for both `NaN` and `Inf` is in my understanding wrong.
If the intention is to use `INT_MAX` for `FP_ILOGBNAN`, the implementation needs to be changed to return a different value for `Inf` (presumably 127 for fp32 and 1023 for fp64, since those can reuse the default codepath).

Note that LLVM internally uses `INT_MIN + 1 (-INT_MAX)` for `ilogb(0)`, `INT_MIN` for `ilogb(NaN)` and `INT_MAX` for `ilogb(Inf)` [0]. This means that any optimization handling these constants and ilogb operation would be incorrect in LLVM.
IMO, the correct action on LLVM side would be to submit a bug to Khronos to either fix their headers or provide clarification wrt `ilogb(Inf)` behaviour.

> Nobody is going to offline compile OpenCL code using the libclc headers. That means that if our implementation wants to leverage libclc behind the scenes, then libclc should use the same definition of this value as the other standard libraries.

Why not? it's fairly easy and straightforward.
What are the requirements for precompiled portability? The specs allow implementations to use different values for `FP_ILOBGNAN` and `FP_ILOGB0`.
afaiu, offline compiled kernels are not expected to work between different implementations. I haven't found a mention of ilogb values in SPIR-V spec.
In this regard clangs opencl headers and libclc are different implementations. Again, I agree that syncing them would be desirable, but keeping two different set of headers is not the right way to do it.

> Yeah, it's different from CPU land where you don't go around compiling against standard library headers from one library, and then linking against another -- except that you can kind of do that too between e.g. glibc and musl (which have matching definitions of this define for what it's worth).

switching libraries at link time requires ABI compatibility, this is no different from CPU libraries, CLC spec only details API.

fwiw: glibc definitions of these `FP_ILOB0` and `FP_ILOGBNAN` vary depending on system configuration [1].
on x86 it's

  #  define FP_ILOGB0     (-2147483647 - 1)
  #  define FP_ILOGBNAN   (-2147483647 - 1)

[2]
while for m68k or is ia64 it's

  #  define FP_ILOGB0     (-2147483647 - 1)
  #  define FP_ILOGBNAN   2147483647

[3, 4]
on other architectures it's

  #  define FP_ILOGB0     (-2147483647)
  #  define FP_ILOGBNAN   2147483647

[5]

So copying IR representations of C programs between those platforms won't work for the same reasons.

[0] https://llvm.org/doxygen/APFloat_8h_source.html#l00229
[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=math/math.h;h=e48860e3915b0ec5c0ae0d594d84432c0568ddc6;hb=HEAD#l190
[2] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/bits/fp-logb.h;h=f180a90754a223547f0d0b965b6fbe6d0132190b;hb=HEAD
[3] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/m68k/m680x0/bits/fp-logb.h;h=bb31eb8d6210033e4be3e8e05bae9a6e09d09e86;hb=HEAD
[4] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/ia64/bits/fp-logb.h;h=45d1bd5e01bb0ecfb9ae7d366f18e315b637c8ca;hb=HEAD
[5] https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/fp-logb.h;h=30effcd52196e7e6a53bde38c66dd6011bbbc3e1;hb=HEAD

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83473/new/

https://reviews.llvm.org/D83473