[OpenCL] Introduce new method for validating OpenCL target

[azabaznov]

Summary

Language options are not available when a target is being created,
thus, a new method is introduced. Also, some refactoring is done,
such as removing OpenCL feature macros setting from TargetInfo.

[Anastasia]

Just a few nitpicks otherwise it looks great!

clang/include/clang/Basic/DiagnosticCommonKinds.td

364

-> warn_opencl_unsupported_core_feature

365

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.

clang/lib/Basic/Targets.cpp

737

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?

clang/lib/Frontend/InitPreprocessor.cpp

610

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.

[azabaznov]

Addressed comments, did some more refactoring. Is it OK to have "2.0" diagnostics for C++ for OpenCL?

[Anastasia]

clang/include/clang/Basic/DiagnosticCommonKinds.td

365

I would prefer:

OpenCL C -> OpenCL version

then it should cover better C++ for OpenCL as it is aligned with OpenCL versions. This is also more consistent with how we report most of the diagnostics with language versions.

But in the future, I want to refactor the diagnostics to have wording about OpenCL C and C++ for OpenCL separately, like we already do in err_opencl_unknown_type_specifier that uses the following helper https://clang.llvm.org/doxygen/classclang_1_1LangOptions.html#a730c1c883221b87b7c3aaec4327b814b

We definitely need a separate pass over diagnostic wording and how we report OpenCL language versions.

clang/include/clang/Basic/OpenCLOptions.h

173

Do you think we could avoid constructing the temporary objects somehow?

I.e. could we just check the condition CLVer >= Avail that is used in the non-static member function directly?

We could then use these helpers in the non-static members perhaps to avoid duplication.

[azabaznov]

Use LangOptions::getOpenCLVersionTuple() to provide diagnostics of OpenCL version

[azabaznov]

clang/include/clang/Basic/OpenCLOptions.h

173

In future I would like to have a file scope variable to to something like:

extern llvm::StringMap<OpenCLOptionInfo> OpenCLOptionsKV = {
#include "OpenCLExtensions.inc"
};

where OpenCLExtensions.inc will be generated from TableGen file. This file-scope variable will be initialized once and contain all the information about OpenCL options. As for now I think using variadics and creating a temporary variable is much more simple than obtaining a full list of arguments and choosing the proper one to call CLVer >= Avail and even for checking core :)

[Anastasia]

LGTM! Thanks.

I have made a minor suggestion to testing that could be addressed in the final commit.

clang/test/Misc/nvptx.unsupported_core.cl

2

We could add a line with -cl-std=CLC++ to check the full wording of the new diagnostic.

[azabaznov]

Add test for C++ for OpenCL diagnostics

[Anastasia]

Cool! Thanks!

clang/include/clang/Basic/DiagnosticCommonKinds.td

def err_unknown_analyzer_checker_or_package : Error<
     "no analyzer checkers or packages are associated with '%0'">;
 def note_suggest_disabling_all_checkers : Note<
     "use -analyzer-disable-all-checks to disable all static analyzer checkers">;
 
 // Poison system directories.
 def warn_poison_system_directories : Warning <
   "include location '%0' is unsafe for cross-compilation">,
   InGroup<DiagGroup<"poison-system-directories">>, DefaultIgnore;
+
+def warn_opencl_unsupported_core_feature : Warning<
+  "%0 is a core feature in %select{OpenCL C|C++ for OpenCL}1 version %2 but not supported on this target">,
+  InGroup<OpenCLCoreFeaturesDiagGroup>, DefaultIgnore;
 }

clang/include/clang/Basic/DiagnosticGroups.td

 Note that the warning is disabled by default, so -Wmax-tokens must be used
 in addition with the pragmas or -fmax-tokens flag to get any warnings.
 }];
 }
 
 def WebAssemblyExceptionSpec : DiagGroup<"wasm-exception-spec">;
 
 def RTTI : DiagGroup<"rtti">;
+
+def OpenCLCoreFeaturesDiagGroup : DiagGroup<"pedantic-core-features">;

clang/include/clang/Basic/DiagnosticParseKinds.td

def warn_pragma_expected_predicate : Warning<
   "expected %select{'enable', 'disable', 'begin' or 'end'|'disable'}0 - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_begin_end_mismatch : Warning<
   "OpenCL extension end directive mismatches begin directive - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_unknown_extension : Warning<
   "unknown OpenCL extension %0 - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_unsupported_extension : Warning<
   "unsupported OpenCL extension %0 - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_extension_is_core : Warning<
-  "OpenCL extension %0 is core feature or supported optional core feature - ignoring">, InGroup<DiagGroup<"pedantic-core-features">>, DefaultIgnore;
+  "OpenCL extension %0 is core feature or supported optional core feature - ignoring">,
+  InGroup<OpenCLCoreFeaturesDiagGroup>, DefaultIgnore;
 
 // OpenCL errors.
 def err_opencl_taking_function_address_parser : Error<
   "taking address of function is not allowed">;
 def err_opencl_logical_exclusive_or : Error<
   "^^ is a reserved operator in OpenCL">;
 
 // C++ for OpenCL.

clang/include/clang/Basic/OpenCLOptions.h

 // Check if OpenCL C version is contained in a given encoded OpenCL C version
 // mask.
 static inline bool isOpenCLVersionContainedInMask(const LangOptions &LO,
                                                   unsigned Mask) {
   auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
   OpenCLVersionID Code = encodeOpenCLVersion(CLVer);
   return Mask & Code;
 }
+
 } // end anonymous namespace
 
 /// OpenCL supported extensions and optional core features
 class OpenCLOptions {
 public:
   struct OpenCLOptionInfo {
     // Does this option have pragma.
     bool WithPragma = false;
public:
   // Disable all extensions
   void disableAll();
 
   friend class ASTWriter;
   friend class ASTReader;
 
   using OpenCLOptionInfoMap = llvm::StringMap<OpenCLOptionInfo>;
 
+  template <typename... Args>
+  static bool isOpenCLOptionCoreIn(const LangOptions &LO, Args &&... args) {
+    return OpenCLOptionInfo(std::forward<Args>(args)...).isCoreIn(LO);
+  }
+
+  template <typename... Args>
+  static bool isOpenCLOptionAvailableIn(const LangOptions &LO,
+                                        Args &&... args) {
+    return OpenCLOptionInfo(std::forward<Args>(args)...).isAvailableIn(LO);
+  }
+
 private:
   // Option is enabled via pragma
   bool isEnabled(llvm::StringRef Ext) const;
 
   OpenCLOptionInfoMap OptMap;
 };
 
 } // end namespace clang
 
 #endif

clang/include/clang/Basic/TargetInfo.h

public:
 
   /// Use the specified unit for FP math.
   ///
   /// \return False on error (invalid unit name).
   virtual bool setFPMath(StringRef Name) {
     return false;
   }
 
+  /// Check if target has a given feature enabled
+  virtual bool hasFeatureEnabled(const llvm::StringMap<bool> &Features,
+                                 StringRef Name) const {
+    return Features.lookup(Name);
+  }
+
   /// Enable or disable a specific target feature;
   /// the feature name must be valid.
   virtual void setFeatureEnabled(llvm::StringMap<bool> &Features,
                                  StringRef Name,
                                  bool Enabled) const {
     Features[Name] = Enabled;
   }
 

 
   /// Whether target defaults to the `power` alignment rules of AIX.
   virtual bool defaultsToAIXPowerAlignment() const { return false; }
 
   /// Set supported OpenCL extensions and optional core features.
   virtual void setSupportedOpenCLOpts() {}
 
   virtual void supportAllOpenCLOpts(bool V = true) {
-#define OPENCLEXTNAME(Ext) getTargetOpts().OpenCLFeaturesMap[#Ext] = V;
+#define OPENCLEXTNAME(Ext)                                                     \
+  setFeatureEnabled(getTargetOpts().OpenCLFeaturesMap, #Ext, V);
 #include "clang/Basic/OpenCLExtensions.def"
   }
 
   /// Set supported OpenCL extensions as written on command line
   virtual void setCommandLineOpenCLOpts() {
     for (const auto &Ext : getTargetOpts().OpenCLExtensionsAsWritten) {
       bool IsPrefixed = (Ext[0] == '+' || Ext[0] == '-');
       std::string Name = IsPrefixed ? Ext.substr(1) : Ext;
       bool V = IsPrefixed ? Ext[0] == '+' : true;
 
       if (Name == "all") {
         supportAllOpenCLOpts(V);
         continue;
       }
 
       getTargetOpts().OpenCLFeaturesMap[Name] = V;
     }
   }
 
-  /// Define OpenCL macros based on target settings and language version
-  void getOpenCLFeatureDefines(const LangOptions &Opts,
-                               MacroBuilder &Builder) const;
-
   /// Get supported OpenCL extensions and optional core features.
   llvm::StringMap<bool> &getSupportedOpenCLOpts() {
     return getTargetOpts().OpenCLFeaturesMap;
   }
 
   /// Get const supported OpenCL extensions and optional core features.
   const llvm::StringMap<bool> &getSupportedOpenCLOpts() const {
     return getTargetOpts().OpenCLFeaturesMap;
const llvm::VersionTuple &getSDKVersion() const {
     return getTargetOpts().SDKVersion;
   }
 
   /// Check the target is valid after it is fully initialized.
   virtual bool validateTarget(DiagnosticsEngine &Diags) const {
     return true;
   }
 
+  /// Check that OpenCL target has valid options setting based on OpenCL
+  /// version.
+  virtual bool validateOpenCLTarget(const LangOptions &Opts,
+                                    DiagnosticsEngine &Diags) const;
+
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
   /// Whether target allows debuginfo types for decl only variables/functions.
   virtual bool allowDebugInfoForExternalRef() const { return false; }
 
 protected:
   /// Copy type and layout related info.
   void copyAuxTarget(const TargetInfo *Aux);

clang/lib/Basic/OpenCLOptions.cpp

 
 void OpenCLOptions::support(llvm::StringRef Ext, bool V) {
   assert(!Ext.empty() && "Extension is empty.");
   assert(Ext[0] != '+' && Ext[0] != '-');
   OptMap[Ext].Supported = V;
 }
 
 OpenCLOptions::OpenCLOptions() {
-#define OPENCL_GENERIC_EXTENSION(Ext, WithPragma, AvailVer, CoreVer, OptVer)   \
-  OptMap.insert_or_assign(                                                     \
-      #Ext, OpenCLOptionInfo{WithPragma, AvailVer, CoreVer, OptVer});
+#define OPENCL_GENERIC_EXTENSION(Ext, ...)                                     \
+  OptMap.insert_or_assign(#Ext, OpenCLOptionInfo{__VA_ARGS__});
 #include "clang/Basic/OpenCLExtensions.def"
 }
 
 void OpenCLOptions::addSupport(const llvm::StringMap<bool> &FeaturesMap,
                                const LangOptions &Opts) {
   for (const auto &F : FeaturesMap) {
     const auto &Name = F.getKey();
     if (F.getValue() && isKnown(Name) && OptMap[Name].isAvailableIn(Opts))

clang/lib/Basic/Targets.cpp

TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
 
   if (!Target->validateTarget(Diags))
     return nullptr;
 
   Target->CheckFixedPointBits();
 
   return Target.release();
 }
-
-/// getOpenCLFeatureDefines - Define OpenCL macros based on target settings
-/// and language version
-void TargetInfo::getOpenCLFeatureDefines(const LangOptions &Opts,
-                                         MacroBuilder &Builder) const {
-  // FIXME: OpenCL options which affect language semantics/syntax
-  // should be moved into LangOptions, thus macro definitions of
-  // such options is better to be done in clang::InitializePreprocessor.
-  auto defineOpenCLExtMacro = [&](llvm::StringRef Name, unsigned AvailVer,
-                                  unsigned CoreVersions,
-                                  unsigned OptionalVersions) {
-    // Check if extension is supported by target and is available in this
-    // OpenCL version
-    auto It = getTargetOpts().OpenCLFeaturesMap.find(Name);
-    if ((It != getTargetOpts().OpenCLFeaturesMap.end()) && It->getValue() &&
-        OpenCLOptions::OpenCLOptionInfo(false, AvailVer, CoreVersions,
-                                        OptionalVersions)
-            .isAvailableIn(Opts))
-      Builder.defineMacro(Name);
+/// validateOpenCLTarget  - Check that OpenCL target has valid
+/// options setting based on OpenCL version.
+bool TargetInfo::validateOpenCLTarget(const LangOptions &Opts,
+                                      DiagnosticsEngine &Diags) const {
+  const llvm::StringMap<bool> &OpenCLFeaturesMap = getSupportedOpenCLOpts();
+
+  auto diagnoseNotSupportedCore = [&](llvm::StringRef Name, auto... OptArgs) {
+    if (OpenCLOptions::isOpenCLOptionCoreIn(Opts, OptArgs...) &&
+        !hasFeatureEnabled(OpenCLFeaturesMap, Name))
+      Diags.Report(diag::warn_opencl_unsupported_core_feature)
+          << Name << Opts.OpenCLCPlusPlus
+          << Opts.getOpenCLVersionTuple().getAsString();
   };
-#define OPENCL_GENERIC_EXTENSION(Ext, WithPragma, Avail, Core, Opt)            \
-  defineOpenCLExtMacro(#Ext, Avail, Core, Opt);
+#define OPENCL_GENERIC_EXTENSION(Ext, ...)                                     \
+  diagnoseNotSupportedCore(#Ext, __VA_ARGS__);
 #include "clang/Basic/OpenCLExtensions.def"
 
-  // Assume compiling for FULL profile
-  Builder.defineMacro("__opencl_c_int64");
+  // For now assume that OpenCL target is always
+  // valid and just provide necessary diagnostics
+  return true;
 }

clang/lib/Basic/Targets/X86.cpp

case 'Y':
     default:
       return false;
     case 'm':
       // 'Ym' is synonymous with 'y'.
     case 'k':
       return Size <= 64;
     case 'z':
       // XMM0/YMM/ZMM0
-      if (FeatureMap.lookup("avx512f"))
+      if (hasFeatureEnabled(FeatureMap, "avx512f"))
         // ZMM0 can be used if target supports AVX512F.
         return Size <= 512U;
-      else if (FeatureMap.lookup("avx"))
+      else if (hasFeatureEnabled(FeatureMap, "avx"))
         // YMM0 can be used if target supports AVX.
         return Size <= 256U;
-      else if (FeatureMap.lookup("sse"))
+      else if (hasFeatureEnabled(FeatureMap, "sse"))
         return Size <= 128U;
       return false;
     case 'i':
     case 't':
     case '2':
       // 'Yi','Yt','Y2' are synonymous with 'x' when SSE2 is enabled.
       if (SSELevel < SSE2)
         return false;
       break;
     }
     break;
   case 'v':
   case 'x':
-    if (FeatureMap.lookup("avx512f"))
+    if (hasFeatureEnabled(FeatureMap, "avx512f"))
       // 512-bit zmm registers can be used if target supports AVX512F.
       return Size <= 512U;
-    else if (FeatureMap.lookup("avx"))
+    else if (hasFeatureEnabled(FeatureMap, "avx"))
       // 256-bit ymm registers can be used if target supports AVX.
       return Size <= 256U;
     return Size <= 128U;
 
   }
 
   return true;
 }

clang/lib/Frontend/CompilerInstance.cpp

if (!getTarget().hasStrictFP() && !getLangOpts().ExpStrictFP) {
     }
     if (getLangOpts().getFPExceptionMode() != LangOptions::FPE_Ignore) {
       getDiagnostics().Report(diag::warn_fe_backend_unsupported_fp_exceptions);
       getLangOpts().setFPExceptionMode(LangOptions::FPE_Ignore);
     }
     // FIXME: can we disable FEnvAccess?
   }
 
+  // We should do it here because target knows nothing about
+  // language options when it's being created.
+  if (getLangOpts().OpenCL)
+    getTarget().validateOpenCLTarget(getLangOpts(), getDiagnostics());
+
   // Inform the target of the language options.
   // FIXME: We shouldn't need to do this, the target should be immutable once
   // created. This complexity should be lifted elsewhere.
   getTarget().adjust(getLangOpts());
 
   // Adjust target options based on codegen options.
   getTarget().adjustTargetOptions(getCodeGenOpts(), getTargetOpts());
 

clang/lib/Frontend/InitPreprocessor.cpp

if (LangOpts.Char8)
     Builder.defineMacro("__cpp_char8_t", "201811L");
   Builder.defineMacro("__cpp_impl_destroying_delete", "201806L");
 
   // TS features.
   if (LangOpts.Coroutines)
     Builder.defineMacro("__cpp_coroutines", "201703L");
 }
 
+/// InitializeOpenCLFeatureTestMacros - Define OpenCL macros based on target
+/// settings and language version
+void InitializeOpenCLFeatureTestMacros(const TargetInfo &TI,
+                                       const LangOptions &Opts,
+                                       MacroBuilder &Builder) {
+  const llvm::StringMap<bool> &OpenCLFeaturesMap = TI.getSupportedOpenCLOpts();
+  // FIXME: OpenCL options which affect language semantics/syntax
+  // should be moved into LangOptions.
+  auto defineOpenCLExtMacro = [&](llvm::StringRef Name, auto... OptArgs) {
+    // Check if extension is supported by target and is available in this
+    // OpenCL version
+    if (TI.hasFeatureEnabled(OpenCLFeaturesMap, Name) &&
+        OpenCLOptions::isOpenCLOptionAvailableIn(Opts, OptArgs...))
+      Builder.defineMacro(Name);
+  };
+#define OPENCL_GENERIC_EXTENSION(Ext, ...)                                     \
+  defineOpenCLExtMacro(#Ext, __VA_ARGS__);
+#include "clang/Basic/OpenCLExtensions.def"
+
+  // Assume compiling for FULL profile
+  Builder.defineMacro("__opencl_c_int64");
+}
+
 static void InitializePredefinedMacros(const TargetInfo &TI,
                                        const LangOptions &LangOpts,
                                        const FrontendOptions &FEOpts,
                                        const PreprocessorOptions &PPOpts,
                                        MacroBuilder &Builder) {
   // Compiler version introspection macros.
   Builder.defineMacro("__llvm__");  // LLVM Backend
   Builder.defineMacro("__clang__"); // Clang Frontend
#undef DEFINE_LOCK_FREE_MACRO
   // Define a macro indicating that the source file is being compiled with a
   // SYCL device compiler which doesn't produce host binary.
   if (LangOpts.SYCLIsDevice) {
     Builder.defineMacro("__SYCL_DEVICE_ONLY__", "1");
   }
 
   // OpenCL definitions.
   if (LangOpts.OpenCL) {
-    TI.getOpenCLFeatureDefines(LangOpts, Builder);
+    InitializeOpenCLFeatureTestMacros(TI, LangOpts, Builder);
 
     if (TI.getTriple().isSPIR())
       Builder.defineMacro("__IMAGE_SUPPORT__");
   }
 
   if (TI.hasInt128Type() && LangOpts.CPlusPlus && LangOpts.GNUMode) {
     // For each extended integer type, g++ defines a macro mapping the
     // index of the type (0 in this case) in some list of extended types

clang/test/Misc/nvptx.unsupported_core.cl

+// RUN: %clang_cc1 -cl-std=CL2.0 -triple nvptx-unknown-unknown -Wpedantic-core-features %s 2> %t
+// RUN: FileCheck --check-prefixes=CHECK-C < %t %s
+// RUN: %clang_cc1 -cl-std=CLC++ -triple nvptx-unknown-unknown -Wpedantic-core-features %s 2> %t
+// RUN: FileCheck --check-prefixes=CHECK-CPP < %t %s
+
+// CHECK-C: cl_khr_3d_image_writes is a core feature in OpenCL C version 2.0 but not supported on this target
+// CHECK-CPP: cl_khr_3d_image_writes is a core feature in C++ for OpenCL version 1.0 but not supported on this target

clang/test/Misc/r600.unsupported_core.cl

+// RUN: %clang_cc1 -cl-std=CL2.0 -triple r600-unknown-unknown -Wpedantic-core-features %s 2> %t
+// RUN: FileCheck < %t %s
+
+// CHECK: cl_khr_byte_addressable_store is a core feature in OpenCL C version 2.0 but not supported on this target
+// CHECK: cl_khr_global_int32_base_atomics is a core feature in OpenCL C version 2.0 but not supported on this target
+// CHECK: cl_khr_global_int32_extended_atomics is a core feature in OpenCL C version 2.0 but not supported on this target
+// CHECK: cl_khr_local_int32_base_atomics is a core feature in OpenCL C version 2.0 but not supported on this target
+// CHECK: cl_khr_local_int32_extended_atomics is a core feature in OpenCL C version 2.0 but not supported on this target
+// CHECK: cl_khr_3d_image_writes is a core feature in OpenCL C version 2.0 but not supported on this target