r269670 - [OpenCL] Add supported OpenCL extensions to target info.

[Liu, Yaxun (Sam) via cfe-commits]

Hi Jeroen,

Thanks for your consideration.

I am OK with removing those extensions which do not affect language or builtin functions. After removing them, they will be unknown to Clang. If a user tries to enable them, a warning will be emitted saying the extension is unknown. 

Anastasia, are you OK with that? Thanks.

Sam

-----Original Message-----
From: Jeroen Ketema [mailto:j.ketema at imperial.ac.uk] 
Sent: Monday, June 6, 2016 5:16 PM
To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>
Cc: Anastasia Stulova <Anastasia.Stulova at arm.com>; Clang Commits <cfe-commits at lists.llvm.org>; nd <nd at arm.com>
Subject: Re: r269670 - [OpenCL] Add supported OpenCL extensions to target info.

Hi Sam,

Thanks baring with me, and thanks a lot for your explanation! I indeed now believe that this is correct; I got somewhat confused by all the macro magic that is going on, and had missed that the enabled extensions are actually stored in a class instance separate from the one with available options.

I still think it would be good if the extensions that only affect the host-side are removed, in particular cl_khr_icd and cl_khr_terminate_context. We have a runtime that indirectly includes both Clang’s OpenCLExtensions.def and the Khronos extension header, and we run into problems when we include the extension header before OpenCLExtensions.def, because the extension header defines macros named cl_khr_icd and cl_khr_terminate_context. I admit this is a somewhat special use-case though.

Thanks again,

 Jeroen

> On 02 Jun 2016, at 21:53, Liu, Yaxun (Sam) <Yaxun.Liu at amd.com> wrote:
> 
> Sorry for the delay.
> 
> In ParsePragma.cpp:
> 
> void Parser::HandlePragmaOpenCLExtension() {  
> assert(Tok.is(tok::annot_pragma_opencl_extension));
>  OpenCLExtData data =
>      OpenCLExtData::getFromOpaqueValue(Tok.getAnnotationValue());
>  unsigned state = data.getInt();
>  IdentifierInfo *ename = data.getPointer();  SourceLocation NameLoc = 
> Tok.getLocation();  ConsumeToken(); // The annotation token.
> 
>  OpenCLOptions &f = Actions.getOpenCLOptions();  auto CLVer = 
> getLangOpts().OpenCLVersion;  auto &Supp = 
> getTargetInfo().getSupportedOpenCLOpts();
>  // OpenCL 1.1 9.1: "The all variant sets the behavior for all 
> extensions,  // overriding all previously issued extension directives, 
> but only if the  // behavior is set to disable."
>  if (state == 0 && ename->isStr("all")) { #define OPENCLEXT(nm) \
>    if (Supp.is_##nm##_supported_extension(CLVer)) \
>      f.nm = 0;
> #include "clang/Basic/OpenCLExtensions.def"
>  }
> #define OPENCLEXT(nm) else if (ename->isStr(#nm)) \
>   if (Supp.is_##nm##_supported_extension(CLVer)) \
>     f.nm = state; \
>   else if (Supp.is_##nm##_supported_core(CLVer)) \
>     PP.Diag(NameLoc, diag::warn_pragma_extension_is_core) << ename; \
>   else \
>     PP.Diag(NameLoc, diag::warn_pragma_unsupported_extension) << 
> ename; #include "clang/Basic/OpenCLExtensions.def"
>  else {
>    PP.Diag(NameLoc, diag::warn_pragma_unknown_extension) << ename;
>    return;
>  }
> }
> 
> Whether an extension is supported is represented by getTargetInfo().getSupportedOpenCLOpts(), which does not change with pragma.
> 
> Whether an extension is enabled is reprented by Actions.getOpenCLOptions(), which changes with pragma.
> 
> test/SemaOpenCL/extensions.cl contains examples of unsupported extensions.
> 
> Sam
> 
> -----Original Message-----
> From: Jeroen Ketema [mailto:j.ketema at imperial.ac.uk]
> Sent: Tuesday, May 31, 2016 6:07 PM
> To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>
> Cc: Anastasia Stulova <Anastasia.Stulova at arm.com>; Clang Commits 
> <cfe-commits at lists.llvm.org>; nd <nd at arm.com>
> Subject: Re: r269670 - [OpenCL] Add supported OpenCL extensions to target info.
> 
> Hi Sam,
> 
>> This commit does not change the initial state of the extensions. An extension is supported is not the same as enabled. At the beginning all extensions are disabled.
> 
> I do not see this reflected in the code at all. Could you please:
> 
> a. Point me to the location where this distinction is made.
> 
> b. Convince me that I cannot enable an extension for a target if that target does not support the extension?
> 
> Jeroen
>