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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia 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)
 
----------------
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?


================
Comment at: clang/lib/Parse/ParsePragma.cpp:785
+      // Therefore, it should never be added by default.
+      Opt.acceptsPragma(Name);
     }
----------------
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...


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

https://reviews.llvm.org/D97052