[PATCH] D83473: libclc: Fix FP_ILOGBNAN definition

[Boris Brezillon via Phabricator via llvm-commits]

bbrezillon added a comment.

In D83473#2143152 <https://reviews.llvm.org/D83473#2143152>, @jvesely wrote:

> What is the problem this patch is trying to address?


Well, the primary goal was to have consistent values in `clang/lib/Headers/opencl-c-base.h` and `libclc/generic/include/clc/float/definitions.h` to avoid the mess when one links against libclc but includes `opencl-c-base.h`.
Not to mention that having 2 conflicting definitions in headers that both lives in the same code base and are supposed to represent the same thing is confusing, to say the least.

> The specs do not mandate these two values to be different.

It might be me misunderstanding the spec here. I read

>   "The value of FP_ILOGB0 shall be either INT_MIN or -INT_MAX. The value of FP_ILOGBNAN shall be either INT_MAX or INT_MIN."

as (`FP_ILOGB0=INT_MIN` and  `FP_ILOGBNAN=INT_MAX`) or (`FP_ILOGB0=-INT_MAX` and  `FP_ILOGBNAN=INT_MIN`).
But you're probably right, there's nothing stating that `FP_ILOGB0` and  `FP_ILOGBNAN` should map to different values, it's just the pattern I've seen in various libc implementations.

> On the more practical side.
>  This patch only changes fp32 implementation to return the new value leaving the fp64 implementation to return `INT_MIN` in both cases.

Oops. The fp64 version should definitely be patched accordingly.

> The implementation now returns `FP_ILOGBNAN` even for `Inf` input, which is not correct.

Hm, nope, it still returns `0x7fffffff`, which is `INT_MAX`. I think you're referring to my comment, where I'm emitting the idea of merging the 2 tests into a single one since `FP_ILOGBNAN` is now also equal to `INT_MAX`, but as mentioned there, I think clarity prevails over optimization (especially since clang might optimize that for us anyway).

> CLC spec doesn't talk about `Inf` inputs, but the libm behaviour is to return `INT_MAX, which might be useful.

Yep, and I didn't change that part.

> If `FP_ILOGBNAN` and `FP_ILOGB0` need to be different it'd be better to use `FP_ILOGBNAN == INT_MIN` and `FP_ILOGB0 == -INT_MAX`.

Except you'd then have a mismatch between `clang/lib/Headers/opencl-c-base.h` and `libclc/generic/include/clc/float/definitions.h`.
So maybe the answer is don't include `opencl-c-base.h` when you link against libclc, but as I mentioned above, the fact that both headers living in the same code base define 2 different values for the same thing is confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83473