Anastasia added a subscriber: cfe-commits.
Anastasia added a comment.
This looks much cleaner than the current flow! Thanks! We should just figure
out a better place for defining the macros (see detailed comment in
Targets.cpp).
I am adding @cfe-commits since it's an important change to be visible to the
community. Other than that I hope @yaxunl and @jvesely would be able to have a
look at the amendment in the setup of AMD and NVPTX targets and notify us in
case they see any concerns.
================
Comment at: clang/include/clang/Basic/OpenCLExtensions.def:46
+
+// Declaration helpers
+#define OPENCL_EXTENSION(ext, avail) OPENCL_GENERIC_EXTENSION(ext, avail, 0U,
0U)
----------------
We should add a spec reference or a note detailing the differences among those
different kinds.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:22
-/// OpenCL supported extensions and optional core features
-class OpenCLOptions {
- struct Info {
- bool Supported; // Is this option supported
- bool Enabled; // Is this option enabled
- unsigned Avail; // Option starts to be available in this OpenCL version
- unsigned Core; // Option becomes (optional) core feature in this OpenCL
- // version
- Info(bool S = false, bool E = false, unsigned A = 100, unsigned C = ~0U)
- :Supported(S), Enabled(E), Avail(A), Core(C){}
- };
- llvm::StringMap<Info> OptMap;
-public:
- /// Check if \c Ext is a recognized OpenCL extension.
- ///
- /// \param Ext - Extension to look up.
- /// \returns \c true if \c Ext is known, \c false otherwise.
- bool isKnown(llvm::StringRef Ext) const {
- return OptMap.find(Ext) != OptMap.end();
- }
+enum OpenCLVersionID : unsigned int {
+ OCL_C_10 = 0x1,
----------------
Let's add a comment to document this new enum.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:36
+// OpenCL C version mask
+class OpenCLVersionEncoderHelper {
+private:
----------------
Looking at the uses, it might be better to just have a helper function that
takes `OpenCLVersion` and `LangOpts` instead of introducing a class that is
only used as temporary.
================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:126
+ // Is supported optional core or core OpenCL feature for OpenCL version \p
+ // CLVer. For supported extension, return false.
+ bool isSupportedCoreOrOptionalCore(llvm::StringRef Ext,
----------------
Btw `CLVer` is not passed here. Same for the other methods above.
================
Comment at: clang/include/clang/Basic/TargetInfo.h:1442
+ virtual void supportAllOpenCLOpts(bool V = true) {
+#define OPENCLEXTNAME(Ext) getTargetOpts().OpenCLFeaturesMap[#Ext] = V;
+#include "clang/Basic/OpenCLExtensions.def"
----------------
Btw here you could just iterate over the elements in the map? This could be
faster than multiple independent look ups into the map. But mainly I think it
makes the flow a bit cleaneras it indicated that you only set elements and not
add them...
================
Comment at: clang/include/clang/Basic/TargetInfo.h:1462
+ void getOpenCLFeatureDefines(const LangOptions &Opts,
+ MacroBuilder &Builder) const;
----------------
This deserves a comment.
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:23
+
+// Is supported as either an extension or an (optional) core feature for
+// OpenCL version \p CLVer.
----------------
I think the comment in the header should be enough, otherwise we will struggle
to keep them in synch.
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:88
+/// Enable or disable support for OpenCL extensions
+/// \param Ext name of the extension optionally prefixed with
+/// '+' or '-'
----------------
I feel this is outdated now?
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:108
+ auto CLVer = Opts.OpenCLCPlusPlus ? 200 : Opts.OpenCLVersion;
+ for (const auto &F : FeaturesMap) {
+ const auto &Name = F.getKey();
----------------
Let's add a comment above saying that this adds options supported for the
targets.
================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:116
+ if (F.getValue().IsCoreIn(Opts))
+ support(F.getKey());
+}
----------------
Btw what if the target doesn't support the feature? Do you think we could
provide an error?
================
Comment at: clang/lib/Basic/Targets.cpp:725
+ if ((It != OpenCLFeaturesMap.end() && It->getValue()) ||
+ OpenCLOptionInfo(AvailVer, CoreVersions, OptionalVersions)
+ .IsCoreIn(Opts))
----------------
I think we should find different place for those now.
Ideally, we should iterate though the map in `OpenCLOptions` and set the define
for the supported ones.
I suggest we move this into `clang::InitializePreprocessor` which means
`Preprocessor` might need a reference to `OpenCLOptions` which is the right
thing to do because it needs to know the features that require the macro, the
same as for `LangOpts`. This means we will need to change the initialization
time of `OpenCLOptions` member in Sema or potentially even reparent it
elsewhere... perhaps to `CompilerInstance` where `LangOpts` and `TargetInfo`
seems to be created already?
================
Comment at: clang/test/Misc/nvptx.languageOptsOpenCL.cl:18
// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple
nvptx64-unknown-unknown -Wpedantic-core-features -DTEST_CORE_FEATURES
+// XFAIL: *
----------------
We shouldn't disable the test, but only change it to check the conformant
behavior i.e. the expected diagnostics for `cl_khr_3d_image_writes` should be
guarded by the OpenCL version.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92277/new/
https://reviews.llvm.org/D92277
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
