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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:62
 
-  /// Check if \c Ext is supported as an (optional) OpenCL core features for
-  /// the given OpenCL version.
-  ///
-  /// \param Ext - Extension to look up.
-  /// \param LO - \c LangOptions specifying the OpenCL version.
-  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
-  bool isSupportedCore(llvm::StringRef Ext, const LangOptions &LO) const {
-    auto E = OptMap.find(Ext);
-    if (E == OptMap.end()) {
-      return false;
-    }
-    // In C++ mode all extensions should work at least as in v2.0.
-    auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
-    auto I = E->getValue();
-    return I.Supported && I.Avail <= CLVer && I.Core != ~0U && CLVer >= I.Core;
-  }
+struct OpenCLOptionInfo {
+  // Option starts to be available in this OpenCL version
----------------
azabaznov wrote:
> Anastasia wrote:
> > I think it would be better to keep it in `OpenCLOptions`, as we don't intend this to be used stand-alone also considering that it's a `struct`? I understand that this is now being used in Targets too but that use should hopefully be eliminated in the future.
> This check is needed to check availability of extensions (see comment in `Targets.cpp`). Or you mean to define it as a nested class in `OpenCLOptions`?
> Or you mean to define it as a nested class in OpenCLOptions?

Exactly.


================
Comment at: clang/lib/Basic/TargetInfo.cpp:360
+    // Set core features based on OpenCL version
+    for (auto CoreExt : clang::getCoreFeatures(Opts))
+      getTargetOpts().OpenCLFeaturesMap[CoreExt] = true;
----------------
azabaznov wrote:
> Anastasia wrote:
> > I still think the target map should be immutable and especially we should not change it silently based on the language compiled even if we have done it before but that caused incorrect behavior i.e. successfully compiling for the architectures that didn't support the features.
> > 
> > If I look at existing targets they already set most of the core features apart from 3d image writes. Perhaps it is reasonable to just drop this code? I don't think it makes the issue worse, in fact, I think it will make the behavior slightly better because now a diagnostic will occur if there is an attempt to use the unsupported feature although the diagnostic won't be the optimal one.  After all it will still remain the responsibility of the user to get the right combination of a language version and a target.
> > 
> > It would be reasonable however to introduce a diagnostic that would report a mismatch between the language version and the hardware support available. We report similar diagnostics in `CompilerInvocation` already. But I don't think we have to do it in this patch because it doesn't introduce any regression. We already have a bug although the behavior of this bug will change. And perhaps if we add `OpenCLOptions` as a part of `LangOpts` at some point this will become straightforward to diagnose. However, I suggest we add information about this issue in a FIXME or perhaps this deserves a clang bug!
> > I still think the target map should be immutable and especially we should not change it silently based on the language compiled
> 
> I'm confused. I think we have agreed to unconditionally support core features for a specific language version. Did I miss something?
> 
> > successfully compiling for the architectures that didn't support the features.
> 
> I like idea providing diagnostics in that case. Something like: "Warning: r600 target doesn't support 
> cl_khr_3d_image_writes which is core in OpenCL C 2.0, consider using OpenCL C 3.0". I also think this should be done in a separate commit.
> 
> > If I look at existing targets they already set most of the core features apart from 3d image writes. Perhaps it is reasonable to just drop this code?
> 
> Oh, I haven't noticed that target set core features. For example //cl_khr_global_int32_base_atomics// is being set by NVPTX and AMDGPU, so I agree that this should be removed from target settings.
It is correct that the core features should be set unconditionally but not in the `TargetInfo`. If core features are used for targets that don't support them then it should not succeed silently as it does now i.e. this means we need to know what is supported by the targets.

Setting target features in `TargetInfo` is correct and should stay.  We should not change them here though because the language version doesn't change the target capabilities. It can either expose or hide them from the user but it should not modify targets. This is why `TargetInfo` is immutable after its creation and this is how it should stay. I think it's better if we remove the code here completely and introduce a diagnostic in the subsequent patches that would just check that the features required in the language version are supported by the target.

If we do this then during the parsing we will only use feature information from `OpenCLOptions` not the targets, but we will know that the target have support of all the features because the check has been performed earlier.


================
Comment at: clang/lib/Basic/Targets.cpp:728
+    // Check if extension is supported by target or is a core feature
+    auto It = getTargetOpts().OpenCLFeaturesMap.find(Name);
+    if ((It != getTargetOpts().OpenCLFeaturesMap.end()) && It->getValue() &&
----------------
azabaznov wrote:
> Anastasia wrote:
> > I still think it would be better if we just iterate over the elements in this map? We should keep the includes of `clang/Basic/OpenCLExtensions.def ` to a minimum.
> This is to check if extension/feature is available (by calling `OpenCLOptionInfo::isAvailableIn(LangOpts)`). For example, //cl_khr_srgb_image_writes// (which is available only in OpenCL C 2.0) can be supported via command  line for OpenCL C 1.2 which is erroneous.  Information about availability of an extension is stored in `OpenCLExtensions.def`.
Ok, we can't do this now because `OpenCLFeaturesMap` is indeed simpler. Hopefully, we can change this in subsequent refactorings if we move `OpenCLOptions` into `LangOpts`.


================
Comment at: clang/lib/Basic/Targets.cpp:730
+    if ((It != getTargetOpts().OpenCLFeaturesMap.end()) && It->getValue() &&
+        OpenCLOptionInfo(AvailVer, CoreVersions, OptionalVersions)
+            .isAvailableIn(Opts))
----------------
azabaznov wrote:
> Anastasia wrote:
> > I think this defines should belong elsewhere, this will eliminate the need to create the temporary object `OpenCLOptionInfo`.  But I appreciate there is only limited amount of refactoring we can do in one patch.
> > 
> > How about we at least add a `FIXME` explaining that some further refactoring is needed to move the macro definition with the rest of macros from `LangOpts`.
> > 
> > 
> >  this will eliminate the need to create the temporary object OpenCLOptionInfo
> 
> This temporary object is needed to check availability of extension (see comment above).
Yes I understand. That's why I believe these macros should no longer be defined in the targets and I think it is very doable to avoid this but it requires some more changes (i.e. moving `OpenCLOptions` into `LangOpts`). So for now we can leave a FIXME comment to explain the issue.  


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

https://reviews.llvm.org/D92277