[PATCH] D97058: [OpenCL] Refactor diagnostic for OpenCL extension/feature

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:364
       for (auto &I : Atomic64BitTypes)
         setOpenCLExtensionForType(I,
             "cl_khr_int64_base_atomics cl_khr_int64_extended_atomics");
----------------
azabaznov wrote:
> Anastasia wrote:
> > I think this should be lifted into the above if statement? Although since we are adding the types conditionally we could even drop this code completely and let the developers just use the types as they are already added and made available by the frontend.
> >  I think this should be lifted into the above if statement?
> 
> No, this is needed to be here as `atomic_double` added above.
> 
> >  Although since we are adding the types conditionally we could even drop this code completely and let the developers just use the types as they are already added and made available by the frontend.
> 
> I'm now worried about diagnostics  with that change:
> 
> ```
> tmp.cl:2:17: error: expected ';' after expression
>     atomic_ulong a;
>                 ^
>                 ;
> tmp.cl:2:5: error: use of undeclared identifier 'atomic_ulong'
>     atomic_ulong a;
>     ^
> tmp.cl:2:18: error: use of undeclared identifier 'a'
>     atomic_ulong a;
>                  ^
> tmp.cl:3:5: error: unknown type name 'atomic_uintptr_t'; did you mean 'atomic_uint'?
>     atomic_uintptr_t u;
>     ^~~~~~~~~~~~~~~~
>     atomic_uint
> note: 'atomic_uint' declared here
> 4 errors generated.
> ```
> Is this expected behaviour when `cl_khr_int64_base_atomics` and 'cl_khr_int64_extended_atomics' not supported? I think we need o be more careful with reserved identifiers. However,  I didn't find explicit wording about it, but I'm pretty sure that all of these atomic types identifiers should be reserved. 
Well, the reserved types are those listed in `6.3.4. Reserved Data Types`.

These are typical examples of library types that are added in the standard headers outside of the compiler even though it was done in the clang source code in this case. Theoretically, they should have been added via an include file but in OpenCL it was supposed to be replaced by the pragma which has never happened for various reasons.

Anyway I think we should just either add the types and let them be used w/o any pragma magic or not add those and allow the identifiers to be reused in the application kernels. This is how atomic builtin functions behave now (e.g. `atomic_init`). The types should not be different.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97058