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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
