[PATCH] D83473: libclc: Fix FP_ILOGBNAN definition

[Jan Vesely via Phabricator via llvm-commits]

jvesely added a comment.

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

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


That combination is not supported and should not be attempted. If you want to link with libclc. you should use clc headers installed by libclc.
I agree that not having multiple conflicting headers is preferable, and I have no idea why clang added their own header instead of using libclc.
I'd say a better solution would be to drop libclc headers entirely and switch the build to use clang's CLC header, or drop clang's CLC header, trying to sync two different locations is just asking for trouble.

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

It doesn't matter if the value is in hex or decimal or a define, The results of `ilogb(Inf)` and `ilogb(NaN)` is now indistinguishable to the caller.
The spec is rather clear that the value of `FP_ILOGBNAN` shall be returned only if the input is NaN.
Since libclc already uses `INT_MAX` for `Inf`, `FP_ILOGBNAN` cannot use the same value.

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

There are potentially tons of other differences between libclc and clang opencl headers, trying to sync them is futile.
libclc historically supported multiple clang versions (the current head can be built using clang 3.9 -> clang 11), I'd strongly prefer to keep it that way until at least clc 1.2 is fully supported, but in the end, it's a question for @tstellar .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83473