[OpenCL] Add support of __opencl_c_read_write_images feature macro

[azabaznov]

Summary

This feature requires support of __opencl_c_images, so diagnostics for that is provided as well

[Anastasia]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10114–10115

Suggesting a slight rewording:
prior to OpenCL C version 2.0 or in version 3.0 without __opencl_c_read_write_images

clang/lib/Basic/Targets.cpp

746

Maybe we should change this comment to something like:

// Validate the feature dependencies for OpenCL C 3.0.

Since it is not exactly about the macros.

752

Maybe we should comment that the first element of the pair is the feature that depends on the feature in the second element?

757

I guess we don't need the compound assignment here?

[azabaznov]

Addressed latest review comments, also refactored the patch by putting diagnostics into OpenCLOptions

[azabaznov]

Fixed naming of new methods, fixed comments, removed redundant comments from source file

[Anastasia]

LGTM! Thanks

[thakis]

clang/lib/Sema/SemaDeclAttr.cpp

7412

Are the parens right here? You probably want

`!S.getLangOpts().OpenCLCPlusPlus && (Ver < 200 || (Ver == 300 && isSupported))`

for the first term, but you have

`!S.getLangOpts().OpenCLCPlusPlus && (Ver < 200) || (Ver == 300 && isSupported)`

Which means the OpenCLCPlusPlus only matters for (Ver < 200) and the (Ver == 300 && ...) term makes things true independent of the OpenCLCPlusPlus check.

If what you currently have is what you want: Remove the parens around (S.getLangOpts().OpenCLVersion < 200) and do instead parenthesize like so: (!S.getLangOpts().OpenCLCPlusPlus && Ver < 200).

[azabaznov]

clang/lib/Sema/SemaDeclAttr.cpp

7412

Thanks. I've submitted the patch: https://reviews.llvm.org/D105892. Please approve.

clang/include/clang/Basic/DiagnosticCommonKinds.td

def warn_poison_system_directories : Warning <
   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;
 
 def err_opencl_extension_and_feature_differs : Error<
   "options %0 and %1 are set to different values">;
+def err_opencl_feature_requires : Error<
+  "feature %0 requires support of %1 feature">;
 }

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_opencl_builtin_pipe_invalid_arg : Error<
   "invalid argument type to function %0 (expecting %1 having %2)">;
 def err_opencl_builtin_pipe_invalid_access_modifier : Error<
   "invalid pipe access modifier (expecting %0)">;
 
 // OpenCL access qualifier
 def err_opencl_invalid_access_qualifier : Error<
   "access qualifier can only be used for pipe and image type">;
 def err_opencl_invalid_read_write : Error<
-  "access qualifier %0 can not be used for %1 %select{|prior to OpenCL version 2.0}2">;
+  "access qualifier %0 can not be used for %1 %select{|prior to OpenCL C version 2.0 or in version 3.0 "
+  "and without __opencl_c_read_write_images feature}2">;
 def err_opencl_multiple_access_qualifiers : Error<
   "multiple access qualifiers">;
 def note_opencl_typedef_access_qualifier : Note<
   "previously declared '%0' here">;
 
 // OpenCL v2.0 s6.12.5 Blocks restrictions
 def err_opencl_block_storage_type : Error<
   "the __block storage type is not permitted">;

clang/include/clang/Basic/OpenCLOptions.h

 #ifndef LLVM_CLANG_BASIC_OPENCLOPTIONS_H
 #define LLVM_CLANG_BASIC_OPENCLOPTIONS_H
 
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/StringMap.h"
 
 namespace clang {
 
+class DiagnosticsEngine;
+class TargetInfo;
+
 namespace {
 // This enum maps OpenCL version(s) into value. These values are used as
 // a mask to indicate in which OpenCL version(s) extension is a core or
 // optional core feature.
 enum OpenCLVersionID : unsigned int {
   OCL_C_10 = 0x1,
   OCL_C_11 = 0x2,
   OCL_C_12 = 0x4,
public:
   }
 
   template <typename... Args>
   static bool isOpenCLOptionAvailableIn(const LangOptions &LO,
                                         Args &&... args) {
     return OpenCLOptionInfo(std::forward<Args>(args)...).isAvailableIn(LO);
   }
 
+  // Diagnose feature dependencies for OpenCL C 3.0. Return false if target
+  // doesn't follow these requirements.
+  static bool diagnoseUnsupportedFeatureDependencies(const TargetInfo &TI,
+                                                     DiagnosticsEngine &Diags);
+
+  // Diagnose that features and equivalent extension are set to same values.
+  // Return false if target doesn't follow these requirements.
+  static bool diagnoseFeatureExtensionDifferences(const TargetInfo &TI,
+                                                  DiagnosticsEngine &Diags);
+
 private:
   // Option is enabled via pragma
   bool isEnabled(llvm::StringRef Ext) const;
 
   OpenCLOptionInfoMap OptMap;
 };
 
 } // end namespace clang
 
 #endif

clang/lib/Basic/OpenCLOptions.cpp

 //===--- OpenCLOptions.cpp---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/OpenCLOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/TargetInfo.h"
 
 namespace clang {
 
 bool OpenCLOptions::isKnown(llvm::StringRef Ext) const {
   return OptMap.find(Ext) != OptMap.end();
 }
 
 bool OpenCLOptions::isAvailableOption(llvm::StringRef Ext,
void OpenCLOptions::addSupport(const llvm::StringMap<bool> &FeaturesMap,
   }
 }
 
 void OpenCLOptions::disableAll() {
   for (auto &Opt : OptMap)
     Opt.getValue().Enabled = false;
 }
 
+bool OpenCLOptions::diagnoseUnsupportedFeatureDependencies(
+    const TargetInfo &TI, DiagnosticsEngine &Diags) {
+  // Feature pairs. First feature in a pair requires the second one to be
+  // supported.
+  static const llvm::StringMap<llvm::StringRef> DependentFeaturesMap = {
+      {"__opencl_c_read_write_images", "__opencl_c_images"}};
+
+  auto OpenCLFeaturesMap = TI.getSupportedOpenCLOpts();
+
+  bool IsValid = true;
+  for (auto &FeaturePair : DependentFeaturesMap)
+    if (TI.hasFeatureEnabled(OpenCLFeaturesMap, FeaturePair.getKey()) &&
+        !TI.hasFeatureEnabled(OpenCLFeaturesMap, FeaturePair.getValue())) {
+      IsValid = false;
+      Diags.Report(diag::err_opencl_feature_requires)
+          << FeaturePair.getKey() << FeaturePair.getValue();
+    }
+  return IsValid;
+}
+
+bool OpenCLOptions::diagnoseFeatureExtensionDifferences(
+    const TargetInfo &TI, DiagnosticsEngine &Diags) {
+  // Extensions and equivalent feature pairs.
+  static const llvm::StringMap<llvm::StringRef> FeatureExtensionMap = {
+      {"cl_khr_fp64", "__opencl_c_fp64"}};
+
+  auto OpenCLFeaturesMap = TI.getSupportedOpenCLOpts();
+
+  bool IsValid = true;
+  for (auto &ExtAndFeat : FeatureExtensionMap)
+    if (TI.hasFeatureEnabled(OpenCLFeaturesMap, ExtAndFeat.getKey()) !=
+        TI.hasFeatureEnabled(OpenCLFeaturesMap, ExtAndFeat.getValue())) {
+      IsValid = false;
+      Diags.Report(diag::err_opencl_extension_and_feature_differs)
+          << ExtAndFeat.getKey() << ExtAndFeat.getValue();
+    }
+  return IsValid;
+}
+
 } // end namespace clang

clang/lib/Basic/Targets.cpp

if (OpenCLOptions::isOpenCLOptionCoreIn(Opts, OptArgs...) &&
       Diags.Report(diag::warn_opencl_unsupported_core_feature)
           << Name << Opts.OpenCLCPlusPlus
           << Opts.getOpenCLVersionTuple().getAsString();
   };
 #define OPENCL_GENERIC_EXTENSION(Ext, ...)                                     \
   diagnoseNotSupportedCore(#Ext, __VA_ARGS__);
 #include "clang/Basic/OpenCLExtensions.def"
 
   // Validate that feature macros are set properly for OpenCL C 3.0.
   // In other cases assume that target is always valid.
   if (Opts.OpenCLCPlusPlus || Opts.OpenCLVersion < 300)
     return true;
 
-  // Feature and corresponding equivalent extension must be set
-  // simultaneously to the same value.
-  for (auto &ExtAndFeat : {std::make_pair("cl_khr_fp64", "__opencl_c_fp64")})
-    if (hasFeatureEnabled(OpenCLFeaturesMap, ExtAndFeat.first) !=
-        hasFeatureEnabled(OpenCLFeaturesMap, ExtAndFeat.second)) {
-      Diags.Report(diag::err_opencl_extension_and_feature_differs)
-          << ExtAndFeat.first << ExtAndFeat.second;
-      return false;
-    }
-
-  return true;
+  return OpenCLOptions::diagnoseUnsupportedFeatureDependencies(*this, Diags) &&
+         OpenCLOptions::diagnoseFeatureExtensionDifferences(*this, Diags);
 }

clang/lib/Sema/SemaDeclAttr.cpp

if (D->hasAttr<OpenCLAccessAttr>()) {
     } else {
       S.Diag(AL.getLoc(), diag::err_opencl_multiple_access_qualifiers)
           << D->getSourceRange();
       D->setInvalidDecl(true);
       return;
     }
   }
 
-  // OpenCL v2.0 s6.6 - read_write can be used for image types to specify that an
-  // image object can be read and written.
-  // OpenCL v2.0 s6.13.6 - A kernel cannot read from and write to the same pipe
-  // object. Using the read_write (or __read_write) qualifier with the pipe
-  // qualifier is a compilation error.
+  // OpenCL v2.0 s6.6 - read_write can be used for image types to specify that
+  // an image object can be read and written. OpenCL v2.0 s6.13.6 - A kernel
+  // cannot read from and write to the same pipe object. Using the read_write
+  // (or __read_write) qualifier with the pipe qualifier is a compilation error.
+  // OpenCL v3.0 s6.8 - For OpenCL C 2.0, or with the
+  // __opencl_c_read_write_images feature, image objects specified as arguments
+  // to a kernel can additionally be declared to be read-write.
   if (const auto *PDecl = dyn_cast<ParmVarDecl>(D)) {
     const Type *DeclTy = PDecl->getType().getCanonicalType().getTypePtr();
     if (AL.getAttrName()->getName().find("read_write") != StringRef::npos) {
       if ((!S.getLangOpts().OpenCLCPlusPlus &&
-           S.getLangOpts().OpenCLVersion < 200) ||
+               (S.getLangOpts().OpenCLVersion < 200) ||
+           (S.getLangOpts().OpenCLVersion == 300 &&
+            !S.getOpenCLOptions().isSupported("__opencl_c_read_write_images",
+                                              S.getLangOpts()))) ||
           DeclTy->isPipeType()) {
         S.Diag(AL.getLoc(), diag::err_opencl_invalid_read_write)
             << AL << PDecl->getType() << DeclTy->isImageType();
         D->setInvalidDecl(true);
         return;
       }
     }
   }

clang/test/Misc/opencl-c-3.0.incorrect_options.cl

-// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=-__opencl_c_fp64,+cl_khr_fp64 %s 2>&1 | FileCheck %s
-// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=+__opencl_c_fp64,-cl_khr_fp64 %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=-__opencl_c_fp64,+cl_khr_fp64 %s 2>&1 | FileCheck -check-prefix=CHECK-FP64 %s
+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=+__opencl_c_fp64,-cl_khr_fp64 %s 2>&1 | FileCheck -check-prefix=CHECK-FP64 %s
+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=+__opencl_c_read_write_images,-__opencl_c_images %s 2>&1 | FileCheck -check-prefix=CHECK-READ-WRITE-IMAGES %s
 
-// CHECK: error: options cl_khr_fp64 and __opencl_c_fp64 are set to different values
+// CHECK-FP64: error: options cl_khr_fp64 and __opencl_c_fp64 are set to different values
+// CHECK-READ-WRITE-IMAGES: error: feature __opencl_c_read_write_images requires support of __opencl_c_images feature

clang/test/SemaOpenCL/access-qualifier.cl

-// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL1.2 %s -cl-ext=-cl_khr_3d_image_writes
-// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL2.0 %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL1.2 %s -cl-ext=-cl_khr_3d_image_writes
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL3.0 %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL3.0 %s -cl-ext=-__opencl_c_read_write_images
 
 typedef image1d_t img1d_ro_default; // expected-note {{previously declared 'read_only' here}}
 
 typedef write_only image1d_t img1d_wo; // expected-note {{previously declared 'write_only' here}}
 typedef read_only image1d_t img1d_ro;
 
-#if __OPENCL_C_VERSION__ >= 200
+#if (__OPENCL_C_VERSION__ == 200) || (__OPENCL_C_VERSION__ == 300 && defined(__opencl_c_read_write_images))
 typedef read_write image1d_t img1d_rw;
 #endif
 
 typedef int Int;
 typedef read_only int IntRO; // expected-error {{access qualifier can only be used for pipe and image type}}
 
 
 void myWrite(write_only image1d_t); // expected-note {{passing argument to parameter here}} expected-note {{passing argument to parameter here}}
 void myRead(read_only image1d_t); // expected-note {{passing argument to parameter here}}
 
-#if __OPENCL_C_VERSION__ >= 200
+#if (__OPENCL_C_VERSION__ == 200) || (__OPENCL_C_VERSION__ == 300 && defined(__opencl_c_read_write_images))
 void myReadWrite(read_write image1d_t);
 #else
-void myReadWrite(read_write image1d_t); // expected-error {{access qualifier 'read_write' can not be used for '__read_write image1d_t' prior to OpenCL version 2.0}}
+void myReadWrite(read_write image1d_t); // expected-error {{access qualifier 'read_write' can not be used for '__read_write image1d_t' prior to OpenCL C version 2.0 or in version 3.0 and without __opencl_c_read_write_images feature}}
 #endif
 
 
 kernel void k1(img1d_wo img) {
   myRead(img); // expected-error {{passing '__private img1d_wo' (aka '__private __write_only image1d_t') to parameter of incompatible type '__read_only image1d_t'}}
 }
 
 kernel void k2(img1d_ro img) {
   myWrite(img); // expected-error {{passing '__private img1d_ro' (aka '__private __read_only image1d_t') to parameter of incompatible type '__write_only image1d_t'}}
 }
 
 kernel void k3(img1d_wo img) {
   myWrite(img);
 }
 
-#if __OPENCL_C_VERSION__ >= 200
+#if (__OPENCL_C_VERSION__ == 200) || (__OPENCL_C_VERSION__ == 300 && defined(__opencl_c_read_write_images))
 kernel void k4(img1d_rw img) {
   myReadWrite(img);
 }
 #endif
 
 kernel void k5(img1d_ro_default img) {
   myWrite(img); // expected-error {{passing '__private img1d_ro_default' (aka '__private __read_only image1d_t') to parameter of incompatible type '__write_only image1d_t'}}
 }

 kernel void k9(read_only int i){} // expected-error{{access qualifier can only be used for pipe and image type}}
 
 kernel void k10(read_only Int img){} // expected-error {{access qualifier can only be used for pipe and image type}}
 
 kernel void k11(read_only write_only image1d_t i){} // expected-error{{multiple access qualifiers}}
 
 kernel void k12(read_only read_only image1d_t i){} // expected-warning {{duplicate 'read_only' declaration specifier}}
 
-#if __OPENCL_C_VERSION__ >= 200
+#if (__OPENCL_C_VERSION__ == 200) || (__OPENCL_C_VERSION__ == 300 && defined(__opencl_c_read_write_images))
 kernel void k13(read_write pipe int i){} // expected-error{{access qualifier 'read_write' can not be used for 'read_only pipe int'}}
 #else
-kernel void k13(__read_write image1d_t i){} // expected-error{{access qualifier '__read_write' can not be used for '__read_write image1d_t' prior to OpenCL version 2.0}}
-#endif
-
-#if __OPENCL_C_VERSION__ >= 200
-void myPipeWrite(write_only pipe int); // expected-note {{passing argument to parameter here}}
-kernel void k14(read_only pipe int p) {
-  myPipeWrite(p); // expected-error {{passing '__private read_only pipe int' to parameter of incompatible type 'write_only pipe int'}}
-}
+kernel void k13(__read_write image1d_t i){} // expected-error{{access qualifier '__read_write' can not be used for '__read_write image1d_t' prior to OpenCL C version 2.0 or in version 3.0 and without __opencl_c_read_write_images feature}}
 #endif
 
 #if __OPENCL_C_VERSION__ < 200
 kernel void test_image3d_wo(write_only image3d_t img) {} // expected-error {{use of type '__write_only image3d_t' requires cl_khr_3d_image_writes support}}
 #endif
 
-#if __OPENCL_C_VERSION__ >= 200
+#if (__OPENCL_C_VERSION__ == 200) || (__OPENCL_C_VERSION__ == 300 && defined(__opencl_c_read_write_images))
 kernel void read_write_twice_typedef(read_write img1d_rw i){} // expected-warning {{duplicate 'read_write' declaration specifier}}
-// expected-note@-74 {{previously declared 'read_write' here}}
+// expected-note@-67 {{previously declared 'read_write' here}}
+#endif
+
+#if OPENCL_C_VERSION__ >= 200
+void myPipeWrite(write_only pipe int); // expected-note {{passing argument to parameter here}}
+kernel void k14(read_only pipe int p) {
+  myPipeWrite(p); // expected-error {{passing '__private read_only pipe int' to parameter of incompatible type 'write_only pipe int'}}
+}
 
 kernel void pipe_ro_twice(read_only read_only pipe int i){} // expected-warning{{duplicate 'read_only' declaration specifier}}
 // Conflicting access qualifiers
 kernel void pipe_ro_twice_tw(read_write read_only read_only pipe int i){} // expected-error{{access qualifier 'read_write' can not be used for 'read_only pipe int'}}
 kernel void pipe_ro_wo(read_only write_only pipe int i){} // expected-error{{multiple access qualifiers}}
 
 typedef read_only pipe int ROPipeInt;
 kernel void pipe_ro_twice_typedef(read_only ROPipeInt i){} // expected-warning{{duplicate 'read_only' declaration specifier}}
 // expected-note@-2 {{previously declared 'read_only' here}}
 
 kernel void pass_ro_typedef_to_wo(ROPipeInt p) {
   myPipeWrite(p); // expected-error {{passing '__private ROPipeInt' (aka '__private read_only pipe int') to parameter of incompatible type 'write_only pipe int'}}
-  // expected-note@-25 {{passing argument to parameter here}}
+  // expected-note@-16 {{passing argument to parameter here}}
 }
 #endif
 
 kernel void read_only_twice_typedef(__read_only img1d_ro i){} // expected-warning {{duplicate '__read_only' declaration specifier}}
 // expected-note@-95 {{previously declared 'read_only' here}}
 
 kernel void read_only_twice_default(read_only img1d_ro_default img){} // expected-warning {{duplicate 'read_only' declaration specifier}}
 // expected-note@-101 {{previously declared 'read_only' here}}
 
 kernel void image_wo_twice(write_only __write_only image1d_t i){} // expected-warning {{duplicate '__write_only' declaration specifier}}
 kernel void image_wo_twice_typedef(write_only img1d_wo i){} // expected-warning {{duplicate 'write_only' declaration specifier}}
 // expected-note@-103 {{previously declared 'write_only' here}}
 

clang/test/SemaOpenCL/unsupported-image.cl

-// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -cl-std=CL3.0 -cl-ext=-__opencl_c_images %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -cl-std=CL3.0 -cl-ext=-__opencl_c_images,-__opencl_c_read_write_images %s
 // RUN: %clang_cc1 -triple spir-unknown-unknown -verify -cl-std=CL3.0 -cl-ext=+__opencl_c_images %s
 
 #ifdef __opencl_c_images
 //expected-no-diagnostics
 #endif
 
 void test1(image1d_t i) {}
 #if !defined(__opencl_c_images)