[OpenCL] Add support of __opencl_c_3d_image_writes feature macro

[azabaznov]

Summary

This feature requires support of __opencl_c_images, so diagnostics for that is provided as well.
Also, ensure that cl_khr_3d_image_writes feature macro is set to the same value.

[azabaznov]

clang/lib/Sema/SemaType.cpp

1732–1734

Note this. Is it OK to refer to API spec here?

[azabaznov]

Change diagnostic output

[Anastasia]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10092

it seems like we keep expanding similar diagnostics which flows against the design policy. Could you try to unify with err_opencl_double_requires_extension or even with err_opencl_requires_extension?

Perhaps we could create a mechanism to print custom strings for extensions that alias features or it can be done in the follow-up patches too?

clang/lib/Sema/SemaType.cpp

1732–1734

I am guessing you want to explain that corresponding features and extensions must be in sync?

It makes sense, but it feels like we are missing something in the kernel language spec then...

[azabaznov]

Unify with err_opencl_requires_extension. Infrastructure for getting aliased extensions seems to messy and doesn't seem worth it yet since there are only two cases of such functionality (fp64 and 3d image writes) :(

[Anastasia]

LGTM! Thanks

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_opencl_variadic_function : Error<
   "invalid prototype, variadic arguments are not allowed in OpenCL">;
 def err_opencl_requires_extension : Error<
   "use of %select{type|declaration}0 %1 requires %2 support">;
 def ext_opencl_double_without_pragma : Extension<
   "Clang permits use of type 'double' regardless pragma if 'cl_khr_fp64' is"
   " supported">;
 def err_opencl_double_requires_extension :  Error<
     "use of type 'double' requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0 support">;
+def err_opencl_3d_image_writes_requires_extensions : Error<
+    "use of type '__write_only image3d_t' requires %select{cl_khr_3d_image_writes|cl_khr_3d_image_writes and "
+    "__opencl_c_3d_image_writes}0 support">;
 def warn_opencl_generic_address_space_arg : Warning<
   "passing non-generic address space pointer to %0"
   " may cause dynamic conversion affecting performance">,
   InGroup<Conversion>, DefaultIgnore;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
 def err_opencl_builtin_pipe_first_arg : Error<
   "first argument to %0 must be a pipe type">;

clang/lib/Basic/OpenCLOptions.cpp

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"}};
+      {"__opencl_c_read_write_images", "__opencl_c_images"},
+      {"__opencl_c_3d_image_writes", "__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"}};
+      {"cl_khr_fp64", "__opencl_c_fp64"},
+      {"cl_khr_3d_image_writes", "__opencl_c_3d_image_writes"}};
 
   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/AMDGPU.h

void setSupportedOpenCLOpts() override {
 
     if (IsAMDGCN) {
       Opts["cl_khr_fp16"] = true;
       Opts["cl_khr_int64_base_atomics"] = true;
       Opts["cl_khr_int64_extended_atomics"] = true;
       Opts["cl_khr_mipmap_image"] = true;
       Opts["cl_khr_mipmap_image_writes"] = true;
       Opts["cl_khr_subgroups"] = true;
-      Opts["cl_khr_3d_image_writes"] = true;
       Opts["cl_amd_media_ops"] = true;
       Opts["cl_amd_media_ops2"] = true;
+
+      Opts["__opencl_c_images"] = true;
+      Opts["__opencl_c_3d_image_writes"] = true;
+      Opts["cl_khr_3d_image_writes"] = true;
     }
   }
 
   LangAS getOpenCLTypeAddrSpace(OpenCLTypeKind TK) const override {
     switch (TK) {
     case OCLTK_Image:
       return LangAS::opencl_constant;
 

clang/lib/Sema/SemaType.cpp

#include "clang/Basic/OpenCLImageTypes.def"
   // FIXME: we want resulting declarations to be marked invalid, but claiming
   // the type is invalid is too strong - e.g. it causes ActOnTypeName to return
   // a null type.
   if (Result->containsErrors())
     declarator.setInvalidType();
 
   if (S.getLangOpts().OpenCL) {
     const auto &OpenCLOptions = S.getOpenCLOptions();
-    StringRef OptName;
+    bool IsOpenCLC30 = (S.getLangOpts().OpenCLVersion == 300);
     // OpenCL C v3.0 s6.3.3 - OpenCL image types require __opencl_c_images
-    // support
+    // support.
+    // OpenCL C v3.0 s6.2.1 - OpenCL 3d image write types requires support
+    // for OpenCL C 2.0, or OpenCL C 3.0 or newer and the
+    // __opencl_c_3d_image_writes feature. OpenCL C v3.0 API s4.2 - For devices
+    // that support OpenCL 3.0, cl_khr_3d_image_writes must be returned when and
+    // only when the optional feature is supported
     if ((Result->isImageType() || Result->isSamplerT()) &&
-        (S.getLangOpts().OpenCLVersion >= 300 &&
-         !OpenCLOptions.isSupported("__opencl_c_images", S.getLangOpts())))
-      OptName = "__opencl_c_images";
-    else if (Result->isOCLImage3dWOType() &&
-             !OpenCLOptions.isSupported("cl_khr_3d_image_writes",
-                                        S.getLangOpts()))
-      OptName = "cl_khr_3d_image_writes";
-
-    if (!OptName.empty()) {
+        (IsOpenCLC30 &&
+         !OpenCLOptions.isSupported("__opencl_c_images", S.getLangOpts()))) {
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_opencl_requires_extension)
-          << 0 << Result << OptName;
+          << 0 << Result << "__opencl_c_images";
+      declarator.setInvalidType();
+    } else if (Result->isOCLImage3dWOType() &&
+               !OpenCLOptions.isSupported("cl_khr_3d_image_writes",
+                                          S.getLangOpts())) {
+      S.Diag(DS.getTypeSpecTypeLoc(),
+             diag::err_opencl_3d_image_writes_requires_extensions)
+          << IsOpenCLC30;
       declarator.setInvalidType();
     }
   }
 
   bool IsFixedPointType = DS.getTypeSpecType() == DeclSpec::TST_accum ||
                           DS.getTypeSpecType() == DeclSpec::TST_fract;
 
   // Only fixed point types can be saturated

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 -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
 
+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=+__opencl_c_3d_image_writes,+__opencl_c_images,-cl_khr_3d_image_writes %s 2>&1 | FileCheck -check-prefix=CHECK-3D-WRITE-IMAGES-DIFF %s
+// RUN: not %clang_cc1 -cl-std=CL3.0 -triple spir-unknown-unknown -cl-ext=+__opencl_c_3d_image_writes,-__opencl_c_images %s 2>&1 | FileCheck -check-prefix=CHECK-3D-WRITE-IMAGES-DEPS %s
+
 // 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
+
+// CHECK-3D-WRITE-IMAGES-DIFF: options cl_khr_3d_image_writes and __opencl_c_3d_image_writes are set to different values
+// CHECK-3D-WRITE-IMAGES-DEPS: error: feature __opencl_c_3d_image_writes requires support of __opencl_c_images feature

clang/test/SemaOpenCL/unsupported-image.cl

-// 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
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -cl-std=CL3.0 -cl-ext=-__opencl_c_images,-__opencl_c_read_write_images,-cl_khr_3d_image_writes,-__opencl_c_3d_image_writes %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -cl-std=CL3.0 -cl-ext=+__opencl_c_images,+__opencl_c_read_write_images,+cl_khr_3d_image_writes,+__opencl_c_3d_image_writes %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -verify -cl-std=CL3.0 -cl-ext=+__opencl_c_images,+__opencl_c_read_write_images,-cl_khr_3d_image_writes,-__opencl_c_3d_image_writes %s
 
-#ifdef __opencl_c_images
+#if defined(__opencl_c_images) && defined(__opencl_c_3d_image_writes)
 //expected-no-diagnostics
 #endif
 
 void test1(image1d_t i) {}
 #if !defined(__opencl_c_images)
 // expected-error@-2{{use of type '__read_only image1d_t' requires __opencl_c_images support}}
 #endif
 

 #if !defined(__opencl_c_images)
 // expected-error@-2{{use of type '__read_only image2d_array_msaa_depth_t' requires __opencl_c_images support}}
 #endif
 
 void test11(sampler_t s) {}
 #if !defined(__opencl_c_images)
 // expected-error@-2{{use of type 'sampler_t' requires __opencl_c_images support}}
 #endif
+
+void test12(write_only image3d_t i) {}
+#if !defined(__opencl_c_images)
+// expected-error@-2{{use of type '__write_only image3d_t' requires __opencl_c_images support}}
+#elif !defined(__opencl_c_3d_image_writes)
+// expected-error@-4{{use of type '__write_only image3d_t' requires cl_khr_3d_image_writes and __opencl_c_3d_image_writes support}}
+#endif