[OpenCL] Override supported OpenCL extensions with -cl-ext option

[asavonic]

Summary

This patch adds a command line option '-cl-ext' to control a set of
supported OpenCL extensions. Option accepts a comma-separated list
of extensions prefixed with '+' or '-'.

It can be used together with a target triple to override support for some
extensions:

// spir target supports all extensions, but we want to disable fp64
clang -cc1 -triple spir-unknown-unknown -cl-ext=-cl_khr_fp64

Special 'all' extension allows to enable or disable all possible
extensions:

// only fp64 will be supported
clang -cc1 -triple spir-unknown-unknown -cl-ext=-all,+cl_khr_fp64

[asavonic]

• Remove unused test case

[Anastasia]

What would be the use case to override the supported extensions for the end user?

The change to set the right extensions based on the target compiled for was to avoid mis-compilations. But adding a user flag to control that could lead to the old problem to reoccur.

[asavonic]

In D23712#520818, @Anastasia wrote:

What would be the use case to override the supported extensions for the end user?

Some extensions may be supported by the platform in general, but not by the
specific version of the OpenCL runtime.

For example, when the platform supports fp64, the OpenCL runtime may not have a
fp64 built-ins implementation. In this case it is better to disable fp64 by the
compiler, rather than get a link error.

Actually, the use case is similar to the -target-feature option, which allows to
control supported CPU features, e.g enable or disable some instruction set.

[Anastasia]

In D23712#524021, @asavonic wrote:

In D23712#520818, @Anastasia wrote:

What would be the use case to override the supported extensions for the end user?

Some extensions may be supported by the platform in general, but not by the
specific version of the OpenCL runtime.

Would triple not be suitable in this case, because they include OS/RT information?

[asavonic]

Hi @Anastasia,

Sorry for my late reply.

What would be the use case to override the supported extensions for the end user?

Some extensions may be supported by the platform in general, but not by the
specific version of the OpenCL runtime.

Would triple not be suitable in this case, because they include OS/RT information?

This can be achieved with a custom triple, yes, but it is not a flexible solution.
We need to create a triple for every combination of extensions. If we want to change it, we need to modify the clang source code and recompile it.

I think it is better to have a default set of extensions (from target triple) and allow user to tweak it without creating one more triple.

So instead of:

clang -cc1 -triple spir-unknown-intel-skl-nofp64-nofp16 <...>

we could write:

clang -cc1 -triple spir-unknown-unknown -cl-ext=-cl_khr_fp64,-cl_khr_fp16 <...>

Both have the same result, but the latter one is more flexible and it can be changed without changing the clang code.

[yaxunl]

I think we need two more tests for concatenating and overriding the option, e.g

-cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64

and

-cl-ext=-cl_khr_fp64,+cl_khr_fp64

include/clang/Basic/OpenCLOptions.h

35 ↗

(On Diff #68677)

nm = Enable;

39 ↗

(On Diff #68677)

It seems Enable should be a local variable.

59 ↗

(On Diff #68677)

nm = Enable;

include/clang/Driver/Options.td

395 ↗

(On Diff #68677)

maybe something like

... Enable or disable specific OpenCL extensions separated by comma. Use 'all' for all extensions.

[asavonic]

Add more test cases and fix minor issues

[asavonic]

include/clang/Basic/OpenCLOptions.h

39 ↗

(On Diff #68677)

This argument is used when Ext is not prefixed by '+' or '-'.

[yaxunl]

I think we need two more tests for concatenating and overriding the option, e.g

-cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64

and

-cl-ext=-cl_khr_fp64,+cl_khr_fp64

include/clang/Basic/OpenCLOptions.h

39 ↗

(On Diff #72143)

Better add a comments for this function about its semantics, i.e., if Ext does not starts with +/-, it is enabled/disabled by \p Enable, otherwise +/- overrides \p Enable, since this is not obvious.

[Anastasia]

include/clang/Basic/OpenCLOptions.h

39 ↗

(On Diff #72143)

Indeed, generally it would be nice to add a comment explaining the purpose of this functions. I don't think the name is descriptive enough.

include/clang/Driver/Options.td

394 ↗

(On Diff #72143)

I would see it as cc1 option instead to avoid confusions on its intension.

395 ↗

(On Diff #72143)

Could we also add a short statement, that +/- are used to turn the extesions on and off.

lib/Basic/Targets.cpp

1882 ↗

(On Diff #72143)

Is this really target specific? I feel this should rather go into common code.

[asavonic]

• Describe OpenCLOptions::set() function
• Move -cl-ext option to cc1
• Reword -cl-ext option help
• Move -cl-ext handling out of target-specific code
• Add two more test cases regarding -cl-ext option

[yaxunl]

LGTM. Thanks!

[joey]

Two minor comments, but otherwise LGTM!

include/clang/Basic/OpenCLOptions.h

33 ↗

(On Diff #73443)

This comment needs to be changed, to reflect that they are now all enabled or disabled.

include/clang/Basic/TargetInfo.h

992 ↗

(On Diff #73443)

Can you put a space around the ':'.

[asavonic]

• Fix comments and code formatting

cfe/trunk/include/clang/Basic/OpenCLOptions.h

 /// \file
 /// \brief Defines the clang::OpenCLOptions class.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_BASIC_OPENCLOPTIONS_H
 #define LLVM_CLANG_BASIC_OPENCLOPTIONS_H
 
+#include "llvm/ADT/StringRef.h"
+
 namespace clang {
 
 /// \brief OpenCL supported extensions and optional core features
 class OpenCLOptions {
 public:
 #define OPENCLEXT(nm) unsigned nm : 1;
 #include "clang/Basic/OpenCLExtensions.def"
 
   OpenCLOptions() {
 #define OPENCLEXT(nm)   nm = 0;
 #include "clang/Basic/OpenCLExtensions.def"
   }
 
-  // Enable all options.
-  void setAll() {
-#define OPENCLEXT(nm)   nm = 1;
+  // Enable or disable all options.
+  void setAll(bool Enable = true) {
+#define OPENCLEXT(nm)   nm = Enable;
+#include "clang/Basic/OpenCLExtensions.def"
+  }
+
+  /// \brief Enable or disable support for OpenCL extensions
+  /// \param Ext name of the extension optionally prefixed with
+  ///        '+' or '-'
+  /// \param Enable used when \p Ext is not prefixed by '+' or '-'
+  void set(llvm::StringRef Ext, bool Enable = true) {
+    assert(!Ext.empty() && "Extension is empty.");
+
+    switch (Ext[0]) {
+    case '+':
+      Enable = true;
+      Ext = Ext.drop_front();
+      break;
+    case '-':
+      Enable = false;
+      Ext = Ext.drop_front();
+      break;
+    }
+
+    if (Ext.equals("all")) {
+      setAll(Enable);
+      return;
+    }
+
+#define OPENCLEXT(nm)       \
+    if (Ext.equals(#nm)) {  \
+      nm = Enable;          \
+    }
 #include "clang/Basic/OpenCLExtensions.def"
   }
 
   // Is supported with OpenCL version \p OCLVer.
 #define OPENCLEXT_INTERNAL(Ext, Avail, ...) \
   bool is_##Ext##_supported(unsigned OCLVer) const { \
     return Ext && OCLVer >= Avail; \
   }

cfe/trunk/include/clang/Basic/TargetInfo.h

public:
   }
 
   /// \brief Whether target allows to overalign ABI-specified prefered alignment
   virtual bool allowsLargerPreferedTypeAlignment() const { return true; }
 
   /// \brief Set supported OpenCL extensions and optional core features.
   virtual void setSupportedOpenCLOpts() {}
 
+  /// \brief Set supported OpenCL extensions as written on command line
+  virtual void setOpenCLExtensionOpts() {
+    for (const auto &Ext : getTargetOpts().OpenCLExtensionsAsWritten) {
+      getTargetOpts().SupportedOpenCLOptions.set(Ext);
+    }
+  }
+
   /// \brief Get supported OpenCL extensions and optional core features.
   OpenCLOptions &getSupportedOpenCLOpts() {
     return getTargetOpts().SupportedOpenCLOptions;
   }
 
   /// \brief Get const supported OpenCL extensions and optional core features.
   const OpenCLOptions &getSupportedOpenCLOpts() const {
       return getTargetOpts().SupportedOpenCLOptions;

cfe/trunk/include/clang/Basic/TargetOptions.h

public:
   /// The list of target specific features to enable or disable -- this should
   /// be a list of strings starting with by '+' or '-'.
   std::vector<std::string> Features;
 
   std::vector<std::string> Reciprocals;
 
   /// Supported OpenCL extensions and optional core features.
   OpenCLOptions SupportedOpenCLOptions;
+
+  /// \brief The list of OpenCL extensions to enable or disable, as written on
+  /// the command line.
+  std::vector<std::string> OpenCLExtensionsAsWritten;
 };
 
 }  // end namespace clang
 
 #endif

cfe/trunk/include/clang/Driver/CC1Options.td

def preamble_bytes_EQ : Joined<["-"], "preamble-bytes=">,
   HelpText<"Assume that the precompiled header is a precompiled preamble "
            "covering the first N bytes of the main file">;
 def token_cache : Separate<["-"], "token-cache">, MetaVarName<"<path>">,
   HelpText<"Use specified token cache file">;
 def detailed_preprocessing_record : Flag<["-"], "detailed-preprocessing-record">,
   HelpText<"include a detailed record of preprocessing actions">;
 
 //===----------------------------------------------------------------------===//
+// OpenCL Options
+//===----------------------------------------------------------------------===//
+
+def cl_ext_EQ : CommaJoined<["-"], "cl-ext=">,
+  HelpText<"OpenCL only. Enable or disable OpenCL extensions. The argument is a comma-separated sequence of one or more extension names, each prefixed by '+' or '-'.">;
+
+//===----------------------------------------------------------------------===//
 // CUDA Options
 //===----------------------------------------------------------------------===//
 
 def fcuda_is_device : Flag<["-"], "fcuda-is-device">,
   HelpText<"Generate code for CUDA device">;
 def fcuda_include_gpubinary : Separate<["-"], "fcuda-include-gpubinary">,
   HelpText<"Incorporate CUDA device-side binary into host object file.">;
 def fcuda_allow_variadic_functions : Flag<["-"], "fcuda-allow-variadic-functions">,

cfe/trunk/lib/Basic/Targets.cpp

TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
   Opts->Features.clear();
   for (const auto &F : Features)
     Opts->Features.push_back((F.getValue() ? "+" : "-") + F.getKey().str());
 
   if (!Target->handleTargetFeatures(Opts->Features, Diags))
     return nullptr;
 
   Target->setSupportedOpenCLOpts();
+  Target->setOpenCLExtensionOpts();
 
   if (!Target->validateTarget(Diags))
     return nullptr;
 
   return Target.release();
 }

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static void ParseTargetArgs(TargetOptions &Opts, ArgList &Args,
   Opts.FPMath = Args.getLastArgValue(OPT_mfpmath);
   Opts.FeaturesAsWritten = Args.getAllArgValues(OPT_target_feature);
   Opts.LinkerVersion = Args.getLastArgValue(OPT_target_linker_version);
   Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
   Opts.Reciprocals = Args.getAllArgValues(OPT_mrecip_EQ);
   // Use the default target triple if unspecified.
   if (Opts.Triple.empty())
     Opts.Triple = llvm::sys::getDefaultTargetTriple();
+  Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
                                         const char *const *ArgBegin,
                                         const char *const *ArgEnd,
                                         DiagnosticsEngine &Diags) {
   bool Success = true;
 

cfe/trunk/test/SemaOpenCL/extensions.cl

 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.1
 
 // Test with a target not supporting fp64.
-// RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -DNOFP64
+// RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -DNOFP64 -DNOFP16
+
+// Test with some extensions enabled or disabled by cmd-line args
+//
+// Target does not support fp64 and fp16 - override it
+// RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+cl_khr_fp64,+cl_khr_fp16
+//
+// Disable or enable all extensions
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -DNOFP64 -DNOFP16
+// RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all
+// RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=+all,-cl_khr_fp64 -DNOFP64
+// RUN: %clang_cc1 %s -triple r600-unknown-unknown -target-cpu r600 -verify -pedantic -fsyntax-only -cl-ext=-all,+cl_khr_fp64 -DNOFP16
+//
+// Concatenating
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-cl_khr_fp64,+cl_khr_fp64
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64 -cl-ext=+cl_khr_fp16 -cl-ext=-cl_khr_fp64 -DNOFP64
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-ext=-all -cl-ext=+cl_khr_fp64,-cl_khr_fp64,+cl_khr_fp16 -DNOFP64
+
+
 
 void f1(double da) { // expected-error {{type 'double' requires cl_khr_fp64 extension}}
   double d; // expected-error {{type 'double' requires cl_khr_fp64 extension}}
   (void) 1.0; // expected-warning {{double precision constant requires cl_khr_fp64}}
 }
 
 #pragma OPENCL EXTENSION cl_khr_fp64 : enable
 #ifdef NOFP64
 // expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp64' - ignoring}}
 #endif
 
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
+#ifdef NOFP16
+// expected-warning@-2{{unsupported OpenCL extension 'cl_khr_fp16' - ignoring}}
+#endif
+
 void f2(void) {
   double d;
 #ifdef NOFP64
 // expected-error@-2{{use of type 'double' requires cl_khr_fp64 extension to be enabled}}
 #endif
 
   (void) 1.0;
 #ifdef NOFP64