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@-28 {{use of type 'atomic_double' (aka '_Atomic(double)')
requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-27 {{use of type 'atomic_intptr_t' (aka
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-28 {{use of type 'atomic_intptr_t' (aka
'_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be
enabled}}
-// expected-error-re@-28 {{use of type 'atomic_uintptr_t' (aka
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-29 {{use of type 'atomic_uintptr_t' (aka
'_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be
enabled}}
-// expected-error-re@-29 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})')
requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})')
requires cl_khr_int64_extended_atomics extension to be enabled}}
-// expected-error-re@-30 {{use of type 'atomic_ptrdiff_t' (aka
'_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
-// expected-error-re@-31 {{use of type 'atomic_ptrdiff_t' (aka
'_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be
enabled}}
+#if __LP64__
+// expected-error-re@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
