[PATCH] D97052: [OpenCL] Prevent adding extension pragma by default

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.


================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:71
+OPENCL_EXTENSION(cl_khr_int64_extended_atomics, true, 100)
+OPENCL_COREFEATURE(cl_khr_3d_image_writes, true, 100, OCL_C_20)
 
----------------
Anastasia wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > azabaznov wrote:
> > > > I think core and optional core features do not require pragma too. So for example 3d image writes or fp64 do not require pragma in OpenCL C 2.0. Isn't that so?
> > > Well to be honest nothing seems to need pragma but we have to accept it for the backward compatibility. :)
> > > 
> > > In case of the core features if pragma would have been useful we would have to still accept it for the backward compatibility even if the feature became core.
> > I'm just wondering if this new field needed in the file to maintain backward compatibility. Maybe we can highlight OpenCL C 3.0 features with some other way? Is it seems that check for name starting with "__opencl_c" is a bad idea?
> Not sure I understand you here but I don't think that we should add extension pragmas any longer at all even for the new extensions. FYI I am planning to add guidelines for that in https://reviews.llvm.org/D97072. Maybe it helps to clarify the idea?
I mean that you could modify `OpenCLOptions::isWithPragma` that it will check if extension/feature was introduced in OpenCL C 3.0. If that's the case then no pragma needed. This new field `pragma` looks redundant as it is set to true only for OpenCL C 3.0 features. However, it may be more cosmetic concern...


================
Comment at: clang/lib/Parse/ParsePragma.cpp:785
+      // Therefore, it should never be added by default.
+      Opt.acceptsPragma(Name);
     }
----------------
Anastasia wrote:
> azabaznov wrote:
> > Anastasia wrote:
> > > svenvh wrote:
> > > > I fail to understand why this is needed, so perhaps this needs a bit more explanation.  Extensions that should continue to support pragmas already have their `WithPragma` field set to `true` via `OpenCLExtensions.def`.  Why do we need to dynamically modify the field?
> > > It is a bit twisty here to be honest. Because we have also introduced the pragma `begin` and `end` that would add pragma `enable`/`disable` by default. So any extension added dynamically using `begin`/`end` would have to accept the pragma `enable`/`disable`. 
> > > 
> > > https://clang.llvm.org/docs/UsersManual.html#opencl-extensions
> > > 
> > > But in the subsequent patches, I hope to remove this because I just don't see where it is useful but it is very confusing.
> > Is it ok not to track this situation here:
> > 
> > ```
> > #pragma OPENCL EXTENSION __opencl_c_feature : begin
> > #pragma OPENCL EXTENSION __opencl_c_feature: end
> > ```
> > 
> > This is some of a corner case, but still...
> I see. I am not sure what should happen here - I guess we should give an error? Although for earlier versions than OpenCL 3.0 this should probably be accepted?
> 
> Perhaps we can create a PR for this for now...
Oh, I think we can ignore this as double underscored identifiers are reserved. It's not expected that users will declare new features.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97052/new/

https://reviews.llvm.org/D97052