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

Mon Mar 1 11:20:55 PST 2021

Anastasia added a comment.

In D97052#2587043 <https://reviews.llvm.org/D97052#2587043>, @azabaznov wrote:

> I see the update regaring this functionality on github issue: https://github.com/KhronosGroup/OpenCL-Docs/issues/82#issuecomment-785496025. Is it a right way to reflect this information in`OpenCLExtensions.def`?

The update only suggests a possible route and I don't want to block on it until there is a final decision. But if that becomes final, it is unlikely we will do something different. All the legacy pragmas will need to be accepted anyway.

================
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:
> > > 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...
You are right that it is only set to `true` for the OpenCL 3.0 features right now but the idea is to provide flexibility to set the pragma for every new extension/feature being added in the future instead of always adding the pragma unconditionally. This is why (I presume) we ended up with pragma for every extension in the first place. No matter that half of them were not used at all. 

Also, there is nothing fundamentally different between extensions and features. The difference is purely wording in the spec. So I prefer us to use the same mechanisms for both.

================
Comment at: clang/lib/Parse/ParsePragma.cpp:785
+      // Therefore, it should never be added by default.
+      Opt.acceptsPragma(Name);
     }
----------------
azabaznov wrote:
> 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.
Yes, I think this is mainly used by tool developers.

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

https://reviews.llvm.org/D97052