[PATCH] D21698: [OpenCL] Allow disabling types and declarations associated with extensions

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 15 inline comments as done.
yaxunl added a comment.

Sorry for the delay. I will update this patch.



================
Comment at: lib/Parse/ParsePragma.cpp:461
+  };
+  typedef std::pair<const IdentifierInfo *, OpenCLExtState> OpenCLExtData;
 }
----------------
Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Could we use PointerIntPair still but with 2 bits?
> > Using PointerIntPair with 2 bits requires IdentifierInfo to be aligned at 4 bytes, however this is not true. IdentifierInfo has no explicit alignment specifier, therefore it can only hold 1 bit in PointerIntPair.
> Based on its structure with having a pointer member I think it's reasonable to assume at least 4 bytes alignment... Although this doesn't seem to affect the performance anyways.
You are right. The alignment of IdentifierInfo should be 8 on most systems.

However in IdentifierTable.h there is


```
// Provide PointerLikeTypeTraits for IdentifierInfo pointers, which
// are not guaranteed to be 8-byte aligned.
template<>
class PointerLikeTypeTraits<clang::IdentifierInfo*> {
public:
  static inline void *getAsVoidPointer(clang::IdentifierInfo* P) {
    return P;
  }
  static inline clang::IdentifierInfo *getFromVoidPointer(void *P) {
    return static_cast<clang::IdentifierInfo*>(P);
  }
  enum { NumLowBitsAvailable = 1 };
};

```
Due to the above code there is only 1 bit available for when using PointerIntPair with IdentifierInfo*. My guess is that alignment of IdentifierInfo is not 8 on certain systems so some one intentionally defined the above code to prevent using PointerIntPair with IdentifierInfo* for more than 1 bit.



================
Comment at: lib/Parse/ParsePragma.cpp:1429
   PP.Lex(Tok);
-  if (Tok.isNot(tok::identifier)) {
-    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_enable_disable);
+  if (Tok.isNot(tok::identifier) && Tok.isNot(tok::kw_register)) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_predicate) << 0;
----------------
Anastasia wrote:
> Not clear why register keyword is here too?
this is old code when I use 'register' to register an extension. will remove it.


================
Comment at: lib/Sema/Sema.cpp:226
                          Context.getAtomicType(Context.UnsignedIntTy));
-      addImplicitTypedef("atomic_long", Context.getAtomicType(Context.LongTy));
-      addImplicitTypedef("atomic_ulong",
-                         Context.getAtomicType(Context.UnsignedLongTy));
+      auto AtomicLongT = Context.getAtomicType(Context.LongTy);
+      addImplicitTypedef("atomic_long", AtomicLongT);
----------------
Anastasia wrote:
> Any reason for changing this too?
To associate atomic long and double types with corresponding extensions so that when the extension is disabled, the associated types are also disabled.


================
Comment at: lib/Sema/Sema.cpp:1558
+  if (Exts.empty())
+    return;
+  auto CanT = T.getCanonicalType().getTypePtr();
----------------
Anastasia wrote:
> Could we then check for an empty string ExtStr as a first function statement instead?
will do


================
Comment at: test/Parser/opencl-atomics-cl20.cl:51
 // expected-error at -28 {{use of type 'atomic_double' (aka '_Atomic(double)') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re at -27 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re at -28 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re at -28 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re at -29 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re at -29 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re at -30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re at -30 {{use of type 'atomic_ptrdiff_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re at -31 {{use of type 'atomic_ptrdiff_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
+#if __LP64__
+// expected-error-re at -28 {{use of type 'atomic_intptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
----------------
Anastasia wrote:
> Why this change?
atomic_intptr_t etc. requires cl_khr_int64_extended_atomics only on 64 bit platforms.

This is a bug which was fixed by this patch.


================
Comment at: test/SemaOpenCL/extension-begin.cl:27
+}
+
+#pragma OPENCL EXTENSION my_ext : disable 
----------------
Anastasia wrote:
> Could you please add case with typedef of a type from an extensions and also using it with qualifiers for the better coverage.
will do


https://reviews.llvm.org/D21698