[PATCH] D100976: [OpenCL] Simplify use of C11 atomic types

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/test/Parser/opencl-atomics-cl20.cl:7-8
 
-#ifdef EXT
-#pragma OPENCL EXTENSION cl_khr_int64_base_atomics:enable
-#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics:enable
-#pragma OPENCL EXTENSION cl_khr_fp64:enable
-#if __OPENCL_C_VERSION__ >= CL_VERSION_1_2
-// expected-warning at -2{{OpenCL extension 'cl_khr_fp64' is core feature or supported optional core feature - ignoring}}
-#endif
+#if defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= CL_VERSION_1_2
+#define LANG_VER_OK
 #endif
----------------
azabaznov wrote:
> Mauby simply //-DLANG_VER_OK// when running tests?
I think defining it in the test source is better for future modifications. Then we could add as many extra RUN lines as possible without the need to pass extra -D flag. Also it is kind of redundant since we already have the language version defined while parsing.


================
Comment at: clang/test/Parser/opencl-atomics-cl20.cl:51-56
+// expected-error at -21 {{expected ';' after expression}}
+// expected-error at -22 {{use of undeclared identifier 's'}}
+// expected-error at -22 {{unknown type name 'atomic_intptr_t'; did you mean 'atomic_int'?}}
+// expected-note@* {{'atomic_int' declared here}}
+// expected-error at -23 {{unknown type name 'atomic_uintptr_t'; did you mean 'atomic_uint'?}}
+// expected-note@* {{'atomic_uint' declared here}}
----------------
azabaznov wrote:
> Well, this is exactly what I was afraid of last time, see https://reviews.llvm.org/D97058#2602677. Is this for sure a right way to go forward? Also, **declared //here//** for implicit type definitions is pretty confusing. Maybe a way the diagnostics showed here is just a bug?
This is a standard diagnostic from clang fixit if the typename is similar to any it can find in a symbol table of valid identifiers.


```
opencl-atomics-cl20.cl:31:3 error: unknown type name 'atomic_intptr_t'; did you mean 'atomic_int'?
atomic_intptr_t ip;
^~~~~~~~~~~~~~~
atomic_int
note: 'atomic_uint' declared here
```

For such implicit typedefs we won't be able to display the source locations as there is no physical source. However this can occur in any kernel code compiled with type names similar to those defined by implicit typedefs i.e. I am not changing this in the patch. 

https://godbolt.org/z/j7vn6d4Kh

But I agree that this is not ideal - we could solve it by not printing such hints at all if source location doesn't exist but this would probably need to be agreed with the community as this should probably be done for all languages. We could create a clang bug to follow up on this? 


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

https://reviews.llvm.org/D100976