[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:
> 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.


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


================
Comment at: clang/test/SemaOpenCL/extension-version.cl:217
+
+// Check that pragmas for the OpenCL 3.0 features are rejected.
+
----------------
azabaznov wrote:
> Again about core or optional core: don't you feel that current diagnostic is good already (https://godbolt.org/z/Kcjdvr)?
First of all with current approach, clang will only provide the diagnostic if an extra warning flag is passed. By default it accepts the pragma silently.

Secondly, to provide consistency clang will either need to know all of the features in the standard even if they are added in the libraries or otherwise, it will provide inconsistent diagnostics - for features added into clang we provide this diagnostic but for the others, we provide the diagnostics that you refer to. I appreciate that we already have the behavior very inconsistent but it is not great.

Another aspect is that the diagnostic we have currently indicates that the pragma is somehow legal (known) and might do something but in fact we never intended it to exist and never intended that frontend would do something with it. So I prefer to have consistency here with the regular diagnostics about the pragmas and indicate that the pragma is unknown. This just makes things simpler for everyone and avoids redundancy in the language too.

FYI this change is generally driving towards deprecation of the extension pragmas overall.


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

https://reviews.llvm.org/D97052