[PATCH] D83473: libclc: Fix FP_ILOGBNAN definition

Sat Jul 11 01:42:35 PDT 2020

bbrezillon added a comment.

>> 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.

Again, I didn't see this clearly stated in the spec. Can you quote the spec or point us to the relevant section?

> 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).

I see several non-CL implementations using the same value for `NaN` and `Inf` [1][2][3]. Again, I'm not saying this applies to CL, but I couldn't find anything in the spec forbidding this collision.

> 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.

Hm, don't you have the same problem when such optimizations happen and the binary is linked with a libc that has different definitions for those values?

[1]https://pubs.opengroup.org/onlinepubs/007908799/xsh/ilogb.html
[2]https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/ilogb-ilogbf-ilogbl2?view=vs-2019
[3]http://redhat.polarhome.com/service/man/?qf=ilogb&tf=2&of=OpenDarwin&sf=3

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83473