[OpenMP] Remove 'stdbool.h' from OpenMP header wrappers

[jhuber6]

Summary

This patch removes the stdbool.h include path from the clang OpenMP
header wrappers. These are used to predeclare the device-side functions
for common headers. The bool header should not be necessary and causes
some problems with redeclarations.

Fixes #57066

[tianshilei1992]

bool is used by a function declaration __ockl_fdot2 in clang/lib/Headers/__clang_hip_libdevice_declares.h, which is included by clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h.

[jhuber6]

Changing bool usage to use _Bool if compiling for C so we don't need to include the extra header for this single declaration.

[tianshilei1992]

I'm fine with the change. @JonChesterfield WDYT?

[JonChesterfield]

We should write down (maybe in the commit message) what this is fixing. The linked bug has someone defining bool as a workaround for msvc which shouldn't be applied when not compiling with msvc, so superficially it looks like this patch works around broken application code. That doesn't seem a good thing.

I don't think this will compile as c89 (not that it would with the bool header either). I don't know what fdot2 does, shall we just cut out the whole function declaration under openmp?

[ivanrodriguez3753]

This looks good to me, and I agree we should document what this is fixing. Any update on if/when this will land?

In my opinion, there's nothing broken about the user code (definitely contrived, though). They didn't ask for stdbool.h so there should not be a redeclaration error. This doesn't seem to be an issue when compiling for NVIDIA or host-only OpenMP, either.

[jhuber6]

In D131639#3749408, @ivanrodriguez3753 wrote:

This looks good to me, and I agree we should document what this is fixing. Any update on if/when this will land?

In my opinion, there's nothing broken about the user code (definitely contrived, though). They didn't ask for stdbool.h so there should not be a redeclaration error. This doesn't seem to be an issue when compiling for NVIDIA or host-only OpenMP, either.

The example code in the bug report suggests the user is defining their own boolean type. I think the C and C++ standard is pretty clear that __ and _[A-Z] identifiers are reserved and may clash at any point if defined by the user. This is typically why these are provided via headers that are guarded appropriately as stdbool.h is. I'm also not sure if this is a problem upstream, or it just reflects some vendor's implementation of it. Maybe others want to comment on this.

[ivanrodriguez3753]

The user didn't define any __ or _[A-Z] identifiers, though?

[jhuber6]

In D131639#3749563, @ivanrodriguez3753 wrote:

The user didn't define any __ or _[A-Z] identifiers, though?

Am I misunderstanding the test input?

/* Visual Studio < 2013 does not have stdbool.h so here it is a replacement: */
#if defined __STDC__ && defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
/* have a C99 compiler */
typedef _Bool bool;
#else
/* do not have a C99 compiler */
typedef unsigned char bool;
#endif

I assumed this was the user defining this on their own.

[ivanrodriguez3753]

They are defining their own bool, which aliases to the built-in _Bool (which is reserved, as you noted with _[A-Z]). I thought bool was fair game unless they included stdbool.h?

[jhuber6]

In D131639#3749582, @ivanrodriguez3753 wrote:

They are defining their own bool, which aliases to the built-in _Bool (which is reserved, as you noted with _[A-Z]). I thought bool was fair game unless they included stdbool.h?

You're right, I was reading it wrong. It should be "legal" to typedef something if no one else has done it.

I'm still wondering if this is really our fault as it's still likely to conflict. The <stdbool.h> header supplies __bool_true_false_are_defined exactly for this reason. Can the user not check this locally to prevent re-declaration? I think it's perfectly reasonable to include system files as part of a toolchain.

[ivanrodriguez3753]

In D131639#3749633, @jhuber6 wrote:

I think it's perfectly reasonable to include system files as part of a toolchain.

I think it comes down to a matter of inconveniencing the user versus the developer. We usually go with the latter. I don't particularly agree with your statement I quoted above, but I can't say I'm an expert in this area. I'll see what others have to say. I've already pulled this into our fork so it doesn't make a difference to us, but I did think it would be useful to get input from the community on whether or not we should pull this in to upstream.

Thanks for your input and responses.

[jdoerfert]

I think the code as is upstream is fine. The test input is problematic. There is no guarantee, or even any argument, that stdbool is not included by the compiler or any header (system or not). If the user writes conflicting cod with the system, that calls for problems down the line.

Long story short, I'd abandon this for now.

[jhuber6]

In D131639#3755866, @jdoerfert wrote:

I think the code as is upstream is fine. The test input is problematic. There is no guarantee, or even any argument, that stdbool is not included by the compiler or any header (system or not). If the user writes conflicting cod with the system, that calls for problems down the line.

Long story short, I'd abandon this for now.

I agree, I will abandon this revision and close the associated bug.

In D131639#3755699, @ivanrodriguez3753 wrote:

In D131639#3749633, @jhuber6 wrote:

I think it's perfectly reasonable to include system files as part of a toolchain.

I think it comes down to a matter of inconveniencing the user versus the developer. We usually go with the latter. I don't particularly agree with your statement I quoted above, but I can't say I'm an expert in this area. I'll see what others have to say. I've already pulled this into our fork so it doesn't make a difference to us, but I did think it would be useful to get input from the community on whether or not we should pull this in to upstream.

Thanks for your input and responses.

I'm glad this helped you fix it on your end, thanks for opening an issue for this.

clang/lib/Headers/__clang_hip_libdevice_declares.h

 __device__ _Float16 __ocml_sin_f16(_Float16);
 __device__ __attribute__((const)) _Float16 __ocml_sqrt_f16(_Float16);
 __device__ __attribute__((const)) _Float16 __ocml_trunc_f16(_Float16);
 __device__ __attribute__((pure)) _Float16 __ocml_pown_f16(_Float16, int);
 
 typedef _Float16 __2f16 __attribute__((ext_vector_type(2)));
 typedef short __2i16 __attribute__((ext_vector_type(2)));
 
+#ifdef __cplusplus
 __device__ __attribute__((const)) float __ockl_fdot2(__2f16 a, __2f16 b,
                                                      float c, bool s);
+#else
+__device__ __attribute__((const)) float __ockl_fdot2(__2f16 a, __2f16 b,
+                                                     float c, _Bool s);
+#endif
 __device__ __attribute__((const)) __2f16 __ocml_ceil_2f16(__2f16);
 __device__ __attribute__((const)) __2f16 __ocml_fabs_2f16(__2f16);
 __device__ __2f16 __ocml_cos_2f16(__2f16);
 __device__ __attribute__((pure)) __2f16 __ocml_exp_2f16(__2f16);
 __device__ __attribute__((pure)) __2f16 __ocml_exp10_2f16(__2f16);
 __device__ __attribute__((pure)) __2f16 __ocml_exp2_2f16(__2f16);
 __device__ __attribute__((const)) __2f16 __ocml_floor_2f16(__2f16);
 __device__ __attribute__((const))

clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h

 
 #pragma omp end declare variant
 
 #ifdef __AMDGCN__
 #pragma omp begin declare variant match(device = {arch(amdgcn)})
 
 // Import types which will be used by __clang_hip_libdevice_declares.h
 #ifndef __cplusplus
-#include <stdbool.h>
 #include <stdint.h>
 #endif
 
 #define __OPENMP_AMDGCN__
 #pragma push_macro("__device__")
 #define __device__
 
 /// Include declarations for libdevice functions.