[PATCH] D92277: [OpenCL] Refactor of targets OpenCL option settings

[Anastasia Stulova via Phabricator via cfe-commits]

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