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

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.


================
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,
----------------
Anastasia wrote:
> Let's add a comment to document this new enum.
I'm going to change comments all over the place in the next patch. Thanks!


================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:36
+// OpenCL C version mask
+class OpenCLVersionEncoderHelper {
+private:
----------------
Anastasia wrote:
> 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.
Ok, that is reasonable. Will change.


================
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"
----------------
Anastasia wrote:
> 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...
> Btw here you could just iterate over the elements in the map

Map is empty at this point. `::supportAllOpenCLOpts` adds all elements which are declared in `OpenCLExtensions.def` into the map.


================
Comment at: clang/lib/Basic/OpenCLOptions.cpp:116
+    if (F.getValue().IsCoreIn(Opts))
+      support(F.getKey());
+}
----------------
Anastasia wrote:
> Btw what if the target doesn't support the feature? Do you think we could provide an error?
I think that if a feature is //core// than a target //must// support it, do I miss something?

However, there is a place for such diagnostics for non-core features already: something similar to `TargetInfo::handleTargetFeatures(std::vector<std::string> &Features, DiagnosticsEngine &Diags)` could be used


================
Comment at: clang/lib/Basic/Targets.cpp:725
+    if ((It != OpenCLFeaturesMap.end() && It->getValue()) ||
+        OpenCLOptionInfo(AvailVer, CoreVersions, OptionalVersions)
+            .IsCoreIn(Opts))
----------------
Anastasia wrote:
> 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?
> I suggest we move this into clang::InitializePreprocessor which means

I think I'm going to introduce `InitializeOpenCLFeatureTestMacros(TargetInfo, LangOptions)` in `InitPreprocessor.cpp`.

> 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

This seems too invasive for me since `CompilerInstance` is a different purpose class; and it already holds `Sema` and `TargetInfo`. `OpenCLOptions` should mainly be used for parsing, so I would like to suggest avoiding using it elsewhere. Furthermore, with the proposed flow `OpenCLOptions.h` can be removed later  and some simple helpers can be used to check the availability of features/extensions.

However, I see your point: we have two identical pieces of code in `TargetInfo::getOpenCLFeatureDefines` and `OpenCLOptions::addSupport`. I think I'll try to get rid of this duplication by transferring setting of core features into `TargetInfo::adjust` which seems pretty right thing to do. What do you think?


================
Comment at: clang/test/Misc/r600.languageOptsOpenCL.cl:26
 // RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple r600-unknown-unknown -Wpedantic-core-features -DTEST_CORE_FEATURES -target-cpu turks
+// XFAIL: *
 
----------------
yaxunl wrote:
> Pls remove XFAIL
Sure. I'll guard this #ifndef check with OpenCL version in existing tests; but however, I think adding new tests with XFAIL for r600 and NVPTX (where #ifndef check fails) seems reasonable to me. What do you think?


================
Comment at: clang/test/Misc/r600.languageOptsOpenCL.cl:113
+// FIXME: this test fails because 3d image writes is core
+// feature in CL 2.0. Target should be using CL 3.0.
 #ifdef cl_khr_3d_image_writes
----------------
yaxunl wrote:
> what's the expected behavior?
> 
> Is cl_khr_3d_image_writes to be always defined for -cl-std=CL2.0?
Yes, 3d image writes is OpenCL C 2.0 core feature.


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

https://reviews.llvm.org/D92277