[PATCH] D83473: libclc: Fix FP_ILOGBNAN definition

Tue Jul 14 11:24:55 PDT 2020

jvesely added a comment.

In D83473#2145805 <https://reviews.llvm.org/D83473#2145805>, @bbrezillon wrote:

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

It's the same section, the wording "The following macros shall expand to integer constant expressions whose values are returned by ilogb(x) if x is zero or NaN, respectively." implies to me that those integer values should not be returned by other inputs, but you're right it's not explicitly stated.

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

yes, the same problem exists for all platforms that provide ilogb behaviour different from the one in LLVM.
This is not the case of llvm using the wrong values internally, but rather using an incorrect constant in constant propagation (afaik it doesn't currently exist).

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

yes. there are multiple different behaviors, depending on the platform. the question is which one should libclc follow. I think returning different values for Inf and NaN is preferable.

I added `ilogb` test to my small test suite[0] and ran it on couple of different platforms:

- clover (current libclc): matches CPU libm behaviour
- ROCm (OpenCL 2.0 AMD-APP.internal (3098.0)): `Incorrect element(1): nan result: 2147483647 correct: -2147483648`
- Intel NEO (OpenCL 2.1 NEO): `Incorrect element(1): nan result: 2147483647 correct: -2147483648`
- Intel CPU (OpenCL 1.2 (Build 117)): `Incorrect element(1): nan result: 2147483647 correct: -2147483648`
- Nvidia (OpenCL 1.2 CUDA 10.2.185): matches CPU libm behaviour

The current libclc implementation was taken from old AMD OpenCL implementation, but I don't have a working setup to test that.

it looks everyone other than clover and Nvidia disagrees with me and uses `INT_MAX` for both `FP_ILOGBNAN` and `ilogb(Inf)`.
I withdraw my objection to the behaviour, I think compatibility is more important in this regard.

can you use the macros instead of hardcoded values in the patch?
thanks

[0] https://github.com/jvesely/ocl_tests/blob/master/ilogb/ilogb.cpp

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83473