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

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

Sam, I ran a few more tests and understood that the overhead mainly comes from extra initialization (in Sema::Initialize()). Therefore, it's more noticeable on a very small kernels.  However, I agree we can probably neglect the overhead as it account for only a couple % of overall time max in Release mode. Sorry, for taking this long time.



> yaxunl wrote in DiagnosticSemaKinds.td:7998
> these two error msgs have different format.
> 
>   def err_type_requires_extension : Error<
>    "use of type %0 requires %1 extension to be enabled">;
> 
> err_type_requires_extension has an extra type name argument.

I guess we could multiplex with select and pass an empty string as a first parameter to the diagnostic? Apart from that, the core part seems to be the same.

> yaxunl wrote in ParsePragma.cpp:461
> 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.

> 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;

Not clear why register keyword is here too?

> 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);

Any reason for changing this too?

> yaxunl wrote in Sema.cpp:1558
> This function can be called grammatically to set extensions for a group of image types, e.g., the extensions associated with image types. It is more convenient to allow empty string here. If empty string is not allowed, it can be diagnosed before calling this function.

Could we then check for an empty string ExtStr as a first function statement instead?

> 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}}

Why this change?

> extension-begin.cl:27
> +}
> +
> +#pragma OPENCL EXTENSION my_ext : disable 

Could you please add case with typedef of a type from an extensions and also using it with qualifiers for the better coverage.

https://reviews.llvm.org/D21698