[PATCH] D101087: [OpenCL] Introduce new method for validating OpenCL target

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Just a few nitpicks otherwise it looks great!



================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:364
+
+def warn_opencl_not_supported_core_feature : Warning<
+  "%0 is a core feature in this OpenCL version but not supported on this target">,
----------------
-> `warn_opencl_unsupported_core_feature`


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:365
+def warn_opencl_not_supported_core_feature : Warning<
+  "%0 is a core feature in this OpenCL version but not supported on this target">,
+  InGroup<OpenCLCoreFeaturesDiagGroup>, DefaultIgnore;
----------------
let's maybe also print OpenCL version explicitly? We already have some logic for it in other diagnostics i.e. `warn_opencl_attr_deprecated_ignored` although I appreciate it seems to be broken for C++ for OpenCL. But we can take care of it separately.


================
Comment at: clang/lib/Basic/Targets.cpp:738
+                                      unsigned OptionalVersions) {
+    if (OpenCLOptions::OpenCLOptionInfo(false, AvailVer, CoreVersions,
                                         OptionalVersions)
----------------
This is just an idea perhaps not for this change - maybe we could introduce static methods for `OpenCLOptions::OpenCLOptionInfo` to allow checking if the features are core or available? Then we can avoid creating temporary objects here and in `InitializeOpenCLFeatureTestMacros`?


================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:610
+  const llvm::StringMap<bool> &OpenCLFeaturesMap = TI.getSupportedOpenCLOpts();
+  // FIXME: OpenCL options which affect language semantics/syntax
+  // should be moved into LangOptions, thus macro definitions of
----------------
I think that the second part of this comment can now be removed



>  thus macro definitions of
>   such options is better to be done in clang::InitializePreprocessor.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101087