[OpenCL] Simplify use of C11 atomic types

[Anastasia]

Summary

Clang's implementation of the extension pragma is broken because it doesn't respect the following requirement from OpenCL Extension spec s1.2:

https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#extensions-overview

disable Behave (including issuing errors and warnings) as if the extension extension_name is not part of the language definition.

This means that extension functionality should not be exposed by default and if extension identifiers are not reserved they should not be recognized by the compiler. This is for example the case for some of the atomic types (i.e. 64 bit types) from extensions.

As fixing the implementation of extension pragma is not feasible and it will affect backward compatibility and require significant rework in the parser this patch simplifies the implementation of pragma by making the behavior as consistent and intuitive as possible.

This change removes the requirement on pragma when atomic types from the extensions are supported because the behavior is not conformant. With this change, the developers can use atomic types from the extensions if they are supported without enabling the pragma because disabling the pragma doesn't do anything useful but only prevents the use of already available identifiers for types and issues extra diagnostics. This makes semantics consistent with the atomic functions that are also available when the extension is supported without any need for the pragma.

This patch does not break backward compatibility since the extension pragma is still supported and it makes the behavior of the compiler less strict by accepting code without extra pragma statements.

Note that there is an ongoing thread to alter the behavior in the spec
https://github.com/KhronosGroup/OpenCL-Docs/issues/82
however this change is orthogonal as it simplifies the current implementation that is not conformant.
Any new functionality can be added later if the behavior in the spec is fixed.

[azabaznov]

clang/test/Parser/opencl-atomics-cl20.cl

7–9

Mauby simply -DLANG_VER_OK when running tests?

67–72

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?

[Anastasia]

clang/test/Parser/opencl-atomics-cl20.cl

7–9

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.

67–72

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?

[mantognini]

This change removes the requirement on pragma when atomic types from the extensions are supported because the behavior is not conformant. With this change, the developers can use atomic types from the extensions if they are supported without enabling the pragma because disabling the pragma doesn't do anything useful but only prevents the use of already available identifiers for types and issues extra diagnostics. This makes semantics consistent with the atomic functions that are also available when the extension is supported without any need for the pragma.

That sounds reasonable to me. Decreasing code complexity and reducing the "surprise-factor" for users by being more consistent is good indeed.

clang/test/Parser/opencl-atomics-cl20.cl

12–14

nit: there are a few tabs in this files you may want to remove before submitting.

50–60

Shouldn't that be

!(defined(cl_khr_int64_extended_atomics) && defined(cl_khr_int64_base_atomics))

for atomic_long and atomic_ulong, and covering double for atomic_double?

Alternatively, if the goal isn't to have 100% coverage in this test of all these variations (which would be fine I believe), then a comment could clarify the intent.

[Anastasia]

clang/test/Parser/opencl-atomics-cl20.cl

50–60

I think for the purpose of this test it is enough that at least one extension is not enabled. So we could add another run line and check for cl_khr_int64_extended_atomics too and also another run line with both although I am not sure it adds a lot into testing at the moment.

We can add a comment, so something like:

Optional type identifiers are not added in earlier version or if at least one of the extensions is not supported. Here we check with `cl_khr_int64_base_atomics` only.

[Anastasia]

clang/test/Parser/opencl-atomics-cl20.cl

50–60

Does it make sense?

[mantognini]

clang/test/Parser/opencl-atomics-cl20.cl

50–60

Yes, I think it does.

[Anastasia]

Added a comment in the test.

[Anastasia]

clang/test/Parser/opencl-atomics-cl20.cl

12–14

There were no tabs or anything weird in the test, but now that I have re-created the patch it doesn't show anything. I wonder why it was showing something earlier...

[mantognini]

LGTM, thanks for the update.

clang/lib/Sema/Sema.cpp

if (getLangOpts().OpenCLCPlusPlus || getLangOpts().OpenCLVersion >= 200) {
       addImplicitTypedef("atomic_int", Context.getAtomicType(Context.IntTy));
       addImplicitTypedef("atomic_uint",
                          Context.getAtomicType(Context.UnsignedIntTy));
       addImplicitTypedef("atomic_float",
                          Context.getAtomicType(Context.FloatTy));
       // OpenCLC v2.0, s6.13.11.6 requires that atomic_flag is implemented as
       // 32-bit integer and OpenCLC v2.0, s6.1.1 int is always 32-bit wide.
       addImplicitTypedef("atomic_flag", Context.getAtomicType(Context.IntTy));
-      auto AtomicSizeT = Context.getAtomicType(Context.getSizeType());
-      addImplicitTypedef("atomic_size_t", AtomicSizeT);
+
 
       // OpenCL v2.0 s6.13.11.6:
       // - The atomic_long and atomic_ulong types are supported if the
       //   cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics
       //   extensions are supported.
       // - The atomic_double type is only supported if double precision
       //   is supported and the cl_khr_int64_base_atomics and
       //   cl_khr_int64_extended_atomics extensions are supported.
       // - If the device address space is 64-bits, the data types
       //   atomic_intptr_t, atomic_uintptr_t, atomic_size_t and
       //   atomic_ptrdiff_t are supported if the cl_khr_int64_base_atomics and
       //   cl_khr_int64_extended_atomics extensions are supported.
+
+      auto AddPointerSizeDependentTypes = [&]() {
+        auto AtomicSizeT = Context.getAtomicType(Context.getSizeType());
+        auto AtomicIntPtrT = Context.getAtomicType(Context.getIntPtrType());
+        auto AtomicUIntPtrT = Context.getAtomicType(Context.getUIntPtrType());
+        auto AtomicPtrDiffT =
+            Context.getAtomicType(Context.getPointerDiffType());
+        addImplicitTypedef("atomic_size_t", AtomicSizeT);
+        addImplicitTypedef("atomic_intptr_t", AtomicIntPtrT);
+        addImplicitTypedef("atomic_uintptr_t", AtomicUIntPtrT);
+        addImplicitTypedef("atomic_ptrdiff_t", AtomicPtrDiffT);
+      };
+
+      if (Context.getTypeSize(Context.getSizeType()) == 32) {
+        AddPointerSizeDependentTypes();
+      }
+
       std::vector<QualType> Atomic64BitTypes;
       if (getOpenCLOptions().isSupported("cl_khr_int64_base_atomics",
                                          getLangOpts()) &&
           getOpenCLOptions().isSupported("cl_khr_int64_extended_atomics",
                                          getLangOpts())) {
         if (getOpenCLOptions().isSupported("cl_khr_fp64", getLangOpts())) {
           auto AtomicDoubleT = Context.getAtomicType(Context.DoubleTy);
           addImplicitTypedef("atomic_double", AtomicDoubleT);
-          setOpenCLExtensionForType(AtomicDoubleT, "cl_khr_fp64");
           Atomic64BitTypes.push_back(AtomicDoubleT);
         }
         auto AtomicLongT = Context.getAtomicType(Context.LongTy);
         auto AtomicULongT = Context.getAtomicType(Context.UnsignedLongTy);
-        auto AtomicIntPtrT = Context.getAtomicType(Context.getIntPtrType());
-        auto AtomicUIntPtrT = Context.getAtomicType(Context.getUIntPtrType());
-        auto AtomicPtrDiffT =
-            Context.getAtomicType(Context.getPointerDiffType());
-
         addImplicitTypedef("atomic_long", AtomicLongT);
         addImplicitTypedef("atomic_ulong", AtomicULongT);
-        addImplicitTypedef("atomic_intptr_t", AtomicIntPtrT);
-        addImplicitTypedef("atomic_uintptr_t", AtomicUIntPtrT);
-        addImplicitTypedef("atomic_ptrdiff_t", AtomicPtrDiffT);
 
-        Atomic64BitTypes.push_back(AtomicLongT);
-        Atomic64BitTypes.push_back(AtomicULongT);
-        if (Context.getTypeSize(AtomicSizeT) == 64) {
-          Atomic64BitTypes.push_back(AtomicSizeT);
-          Atomic64BitTypes.push_back(AtomicIntPtrT);
-          Atomic64BitTypes.push_back(AtomicUIntPtrT);
-          Atomic64BitTypes.push_back(AtomicPtrDiffT);
+
+        if (Context.getTypeSize(Context.getSizeType()) == 64) {
+          AddPointerSizeDependentTypes();
         }
       }
-
-      for (auto &I : Atomic64BitTypes)
-        setOpenCLExtensionForType(I,
-            "cl_khr_int64_base_atomics cl_khr_int64_extended_atomics");
     }
 
 
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext)                                      \
   if (getOpenCLOptions().isSupported(#Ext, getLangOpts())) {                   \
     addImplicitTypedef(#ExtType, Context.Id##Ty);                              \
     setOpenCLExtensionForType(Context.Id##Ty, #Ext);                           \
   }

clang/test/Parser/opencl-atomics-cl20.cl

 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20
-// RUN: %clang_cc1 %s -triple spir64-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -DCL20 -DEXT -Wpedantic-core-features
-
-#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@-2{{OpenCL extension 'cl_khr_fp64' is core feature or supported optional core feature - ignoring}}
-#endif
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -cl-ext=-cl_khr_int64_base_atomics
+// RUN: %clang_cc1 %s -triple spir64-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0
+// RUN: %clang_cc1 %s -triple spir64-unknown-unknown -verify -fsyntax-only -cl-std=CLC++
+// RUN: %clang_cc1 %s -triple spir64-unknown-unknown -verify -fsyntax-only -cl-std=CL2.0 -cl-ext=-cl_khr_int64_base_atomics
+
+#if defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= CL_VERSION_1_2
+#define LANG_VER_OK
 #endif
 
 void atomic_types_test() {
 // OpenCL v2.0 s6.13.11.6 defines supported atomic types.
+
+// Non-optional types
   atomic_int i;
   atomic_uint ui;
+  atomic_float f;
+  atomic_flag fl;
+#if !defined(LANG_VER_OK)
+// expected-error@-5 {{use of undeclared identifier 'atomic_int'}}
+// expected-error@-5 {{use of undeclared identifier 'atomic_uint'}}
+// expected-error@-5 {{use of undeclared identifier 'atomic_float'}}
+// expected-error@-5 {{use of undeclared identifier 'atomic_flag'}}
+#endif
+
+// Optional types
   atomic_long l;
   atomic_ulong ul;
-  atomic_float f;
   atomic_double d;
-  atomic_flag fl;
+  atomic_size_t s;
   atomic_intptr_t ip;
   atomic_uintptr_t uip;
-  atomic_size_t s;
   atomic_ptrdiff_t pd;
-// OpenCL v2.0 s6.13.11.8, _Atomic type specifier and _Atomic type qualifier
-// are not supported by OpenCL.
-  _Atomic int i; // expected-error {{use of undeclared identifier '_Atomic'}}
-}
-#ifndef CL20
-// expected-error@-16 {{use of undeclared identifier 'atomic_int'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_uint'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_long'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_ulong'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_float'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_double'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_flag'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_intptr_t'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_uintptr_t'}}
-// expected-error@-16 {{use of undeclared identifier 'atomic_size_t'}}
+// Optional type identifiers are not added in earlier version or if at least
+// one of the extensions is not supported. Here we check with
+// `cl_khr_int64_base_atomics` only.
+#if !defined(LANG_VER_OK) || !defined(cl_khr_int64_base_atomics)
+// expected-error@-11 {{use of undeclared identifier 'atomic_long'}}
+// expected-error@-11 {{use of undeclared identifier 'atomic_ulong'}}
+// expected-error@-11 {{use of undeclared identifier 'atomic_double'}}
+#if defined(LANG_VER_OK)
+// expected-error@-15 {{expected ';' after expression}}
+// expected-error@-16 {{use of undeclared identifier 'l'}}
+// expected-error@-16 {{expected ';' after expression}}
+// expected-error@-17 {{use of undeclared identifier 'ul'}}
+#endif
+#if !defined(LANG_VER_OK) || defined(__SPIR64__)
+// expected-error@-18 {{use of undeclared identifier 'atomic_size_t'}}
 // expected-error@-16 {{use of undeclared identifier 'atomic_ptrdiff_t'}}
-#elif !EXT
-// expected-error@-26 {{use of type 'atomic_long' (aka '_Atomic(long)') requires cl_khr_int64_base_atomics support}}
-// expected-error@-27 {{use of type 'atomic_long' (aka '_Atomic(long)') requires cl_khr_int64_extended_atomics support}}
-// expected-error@-27 {{use of type 'atomic_ulong' (aka '_Atomic(unsigned long)') requires cl_khr_int64_base_atomics support}}
-// expected-error@-28 {{use of type 'atomic_ulong' (aka '_Atomic(unsigned long)') requires cl_khr_int64_extended_atomics support}}
-// expected-error@-27 {{use of type 'atomic_double' (aka '_Atomic(double)') requires cl_khr_int64_base_atomics support}}
-// expected-error@-28 {{use of type 'atomic_double' (aka '_Atomic(double)') requires cl_khr_int64_extended_atomics support}}
-#if __LP64__
-// expected-error-re@-28 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics support}}
-// expected-error-re@-29 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics support}}
-// expected-error-re@-29 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics support}}
-// expected-error-re@-30 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics support}}
-// expected-error-re@-30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics support}}
-// expected-error-re@-31 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics support}}
-// expected-error-re@-31 {{use of type 'atomic_ptrdiff_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics support}}
-// expected-error-re@-32 {{use of type 'atomic_ptrdiff_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics support}}
+#if !defined(LANG_VER_OK)
+// expected-error@-20 {{use of undeclared identifier 'atomic_intptr_t'}}
+// expected-error@-20 {{use of undeclared identifier 'atomic_uintptr_t'}}
+#else
+// expected-error@-24 {{expected ';' after expression}}
+// expected-error@-25 {{use of undeclared identifier 's'}}
+// expected-error@-25 {{unknown type name 'atomic_intptr_t'; did you mean 'atomic_int'?}}
+// expected-note@* {{'atomic_int' declared here}}
+// expected-error@-26 {{unknown type name 'atomic_uintptr_t'; did you mean 'atomic_uint'?}}
+// expected-note@* {{'atomic_uint' declared here}}
+#endif
 #endif
 #endif
 
-#ifdef CL20
+// OpenCL v2.0 s6.13.11.8, _Atomic type specifier and _Atomic type qualifier
+// are not supported by OpenCL.
+  _Atomic int i;
+#ifdef __OPENCL_C_VERSION__
+// expected-error@-2 {{use of undeclared identifier '_Atomic'}}
+#else
+ // expected-error@-4 {{unknown type name '_Atomic'}}
+#endif
+}
+
+#if defined(LANG_VER_OK)
+int atomic_uint; //expected-error{{redefinition of 'atomic_uint' as different kind of symbol}}
 void foo(atomic_int * ptr) {}
 void atomic_ops_test() {
   atomic_int i;
   foo(&i);
 // OpenCL v2.0 s6.13.11.8, arithemtic operations are not permitted on atomic types.
   i++; // expected-error {{invalid argument type '__private atomic_int' (aka '__private _Atomic(int)') to unary expression}}
   i = 1; // expected-error {{atomic variable can be assigned to a variable only in global address space}}
   i += 1; // expected-error {{invalid operands to binary expression ('__private atomic_int' (aka '__private _Atomic(int)') and 'int')}}
   i = i + i; // expected-error {{invalid operands to binary expression ('__private atomic_int' (aka '__private _Atomic(int)') and '__private atomic_int')}}
 }
+#else
+__constant int atomic_uint = 1;
 #endif