[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked an inline comment as done.
Anastasia added inline comments.


================
Comment at: test/Headers/opencl-c-header.cl:57-65
 char f(char x) {
-#if __OPENCL_C_VERSION__ != CL_VERSION_2_0
+#if !defined(__OPENCL_CPP_VERSION__) && (__OPENCL_C_VERSION__ != CL_VERSION_2_0)
   return convert_char_rte(x);
 
 #else //__OPENCL_C_VERSION__
   ndrange_t t;
   return ctz(x);
----------------
bader wrote:
> This test looks strange.
> It checks that convert_char_rte is compiled if OpenCL version is not 2.0, but AFAIK it's not deprecated in OpenCL C 2.0, so I don't know why test is written in this way (i.e. ifdef-else-endif).
> 
> I think checks should look like this:
> ```
> char f(char x) {
> // Check check OpenCL C 2.0 and OpenCL C++ functionality 
> #if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)
>   ndrange_t t;
>   x = ctz(x);
> #endif
>   return convert_char_rte(x);
> }
> ```
> 
> Probably it's better to fix separately.
Ok, I think the idea was just to have 2 unique outputs to be checked that we can detect CL2.0 or earlier. But I guess we can do the same with the structure you are suggesting too. I don't mind changing it.

I have to say the header testing is so ad-hoc at the moment and I would like to improve it but because it's so costly we probably need this to be done conditionally. I was thinking to activate it with an extra flag in cmake. Anyway, this is something for the future.


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

https://reviews.llvm.org/D59486