[OpenCL] Fix optional image types

[Anastasia]

Summary

This change allows the use of identifiers for image types from cl_khr_gl_msaa_sharing freely in the kernel code if the extension is not supported since they are not in the list of the reserved identifiers:
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#keywords

This change also removed the need for pragma for the types in the extensions since the spec doesn't require the pragma uses. For example, there is the following wording for 3d image writes:

The behavior of write_imagef, write_imagei and write_imageui for image objects with image_channel_data_type values not specified in the description above or with (x, y, z) coordinate values that are not in the range [0, image width-1], [0, image height-1], and [0, image depth-1], respectively, is undefined.

Requires support for OpenCL C 2.0, or OpenCL C 3.0 or newer and the __opencl_c_3d_image_writes feature, or the cl_khr_3d_image_writes extension.

[azabaznov]

Generally looks good to me, but maybe a test needed, see a comment. Thanks! But I'm still not sure about completely removing pragmas for type declarations (see https://reviews.llvm.org/D100980#2719196).

clang/include/clang/Basic/OpenCLImageTypes.def

68

Maybe we should add a test to check that` image3d_t` is reserved?

[Anastasia]

clang/include/clang/Basic/OpenCLImageTypes.def

68

Do you mean something like:

typedef int image3d_t;

And then check that this gives an error?

[azabaznov]

clang/include/clang/Basic/OpenCLImageTypes.def

68

Yes, exactly. But currently there is already an expected behaviour: https://godbolt.org/z/4afjf5brn. I don't think that your patch changes that, but still, it's good to add a test since you are removing cl_khr_3d_image_writes from image descriptions.

[Anastasia]

Test all image types as identifiers

[Anastasia]

clang/include/clang/Basic/OpenCLImageTypes.def

68

Improving testing is always desirable. I have added testing for all image types that are implemented in clang now.

I don't think that your patch changes that, but still, it's good to add a test since you are removing cl_khr_3d_image_writes from image descriptions.

Technically I am moving it elsewhere so it is no longer needed here. :)

[Anastasia]

Improved negative testing for cl_khr_gl_msaa_sharing

[azabaznov]

LGTM. Thanks!

clang/include/clang/Basic/OpenCLImageTypes.def

 IMAGE_WRITE_TYPE(image2d, OCLImage2d, "")
 IMAGE_WRITE_TYPE(image2d_array, OCLImage2dArray, "")
 IMAGE_WRITE_TYPE(image2d_depth, OCLImage2dDepth, "")
 IMAGE_WRITE_TYPE(image2d_array_depth, OCLImage2dArrayDepth, "")
 IMAGE_WRITE_TYPE(image2d_msaa, OCLImage2dMSAA, "cl_khr_gl_msaa_sharing")
 IMAGE_WRITE_TYPE(image2d_array_msaa, OCLImage2dArrayMSAA, "cl_khr_gl_msaa_sharing")
 IMAGE_WRITE_TYPE(image2d_msaa_depth, OCLImage2dMSAADepth, "cl_khr_gl_msaa_sharing")
 IMAGE_WRITE_TYPE(image2d_array_msaa_depth, OCLImage2dArrayMSAADepth, "cl_khr_gl_msaa_sharing")
-IMAGE_WRITE_TYPE(image3d, OCLImage3d, "cl_khr_3d_image_writes")
+IMAGE_WRITE_TYPE(image3d, OCLImage3d, "")
 
 IMAGE_READ_WRITE_TYPE(image1d, OCLImage1d, "")
 IMAGE_READ_WRITE_TYPE(image1d_array, OCLImage1dArray, "")
 IMAGE_READ_WRITE_TYPE(image1d_buffer, OCLImage1dBuffer, "")
 IMAGE_READ_WRITE_TYPE(image2d, OCLImage2d, "")
 IMAGE_READ_WRITE_TYPE(image2d_array, OCLImage2dArray, "")
 IMAGE_READ_WRITE_TYPE(image2d_depth, OCLImage2dDepth, "")
 IMAGE_READ_WRITE_TYPE(image2d_array_depth, OCLImage2dArrayDepth, "")

clang/lib/Parse/ParseDecl.cpp

while (1) {
     // the token as an identifier.
     if (getLangOpts().MSVCCompat && Tok.is(tok::kw__Atomic) &&
         DS.getStorageClassSpec() == clang::DeclSpec::SCS_typedef &&
         !DS.hasTypeSpecifier() && GetLookAheadToken(1).is(tok::less))
       Tok.setKind(tok::identifier);
 
     SourceLocation Loc = Tok.getLocation();
 
+    // Helper for image types in OpenCL.
+    auto handleOpenCLImageKW = [&] (StringRef Ext, TypeSpecifierType ImageTypeSpec) {
+      // Check if the image type is supported and otherwise turn the keyword into an identifier
+      // because image types from extensions are not reserved identifiers.
+      if (!StringRef(Ext).empty() && !getActions().getOpenCLOptions().isSupported(Ext, getLangOpts())) {
+        Tok.getIdentifierInfo()->revertTokenIDToIdentifier();
+        Tok.setKind(tok::identifier);
+        return false;
+      }
+      isInvalid = DS.SetTypeSpecType(ImageTypeSpec, Loc, PrevSpec, DiagID, Policy);
+      return true;
+    };
+
     switch (Tok.getKind()) {
     default:
     DoneWithDeclSpec:
       if (!AttrsLastTime)
         ProhibitAttributes(attrs);
       else {
         // Reject C++11 attributes that appertain to decl specifiers as
         // we don't support any C++11 attributes that appertain to decl
case tok::kw_pipe:
         // OpenCL 2.0 and later define this keyword. OpenCL 1.2 and earlier
         // should support the "pipe" word as identifier.
         Tok.getIdentifierInfo()->revertTokenIDToIdentifier();
         Tok.setKind(tok::identifier);
         goto DoneWithDeclSpec;
       }
       isInvalid = DS.SetTypePipe(true, Loc, PrevSpec, DiagID, Policy);
       break;
-#define GENERIC_IMAGE_TYPE(ImgType, Id) \
+// We only need to enumerate each image type once.
+#define IMAGE_READ_WRITE_TYPE(Type, Id, Ext)
+#define IMAGE_WRITE_TYPE(Type, Id, Ext)
+#define IMAGE_READ_TYPE(ImgType, Id, Ext) \
     case tok::kw_##ImgType##_t: \
-    isInvalid = DS.SetTypeSpecType(DeclSpec::TST_##ImgType##_t, Loc, PrevSpec, \
-                                   DiagID, Policy); \
+      if (!handleOpenCLImageKW(Ext, DeclSpec::TST_##ImgType##_t)) \
+        goto DoneWithDeclSpec; \
       break;
 #include "clang/Basic/OpenCLImageTypes.def"
     case tok::kw___unknown_anytype:
       isInvalid = DS.SetTypeSpecType(TST_unknown_anytype, Loc,
                                      PrevSpec, DiagID, Policy);
       break;
 
     // class-specifier:
     case tok::kw_class:

clang/lib/Sema/Sema.cpp

if (getLangOpts().OpenCLCPlusPlus || getLangOpts().OpenCLVersion >= 200) {
 
       for (auto &I : Atomic64BitTypes)
         setOpenCLExtensionForType(I,
             "cl_khr_int64_base_atomics cl_khr_int64_extended_atomics");
     }
 
     setOpenCLExtensionForType(Context.DoubleTy, "cl_khr_fp64");
 
-#define GENERIC_IMAGE_TYPE_EXT(Type, Id, Ext) \
-    setOpenCLExtensionForType(Context.Id, Ext);
-#include "clang/Basic/OpenCLImageTypes.def"
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext)                                      \
   if (getOpenCLOptions().isSupported(#Ext, getLangOpts())) {                   \
     addImplicitTypedef(#ExtType, Context.Id##Ty);                              \
     setOpenCLExtensionForType(Context.Id##Ty, #Ext);                           \
   }
 #include "clang/Basic/OpenCLExtensionTypes.def"
   }
 

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 && Result->isOCLImage3dWOType() &&
+      !S.getOpenCLOptions().isSupported("cl_khr_3d_image_writes", S.getLangOpts())) {
+        S.Diag(DS.getTypeSpecTypeLoc(), diag::err_opencl_requires_extension)
+        << 0 << Result << "cl_khr_3d_image_writes";
+        declarator.setInvalidType();
+  }
+
   if (S.getLangOpts().OpenCL &&
       S.checkOpenCLDisabledTypeDeclSpec(DS, Result))
     declarator.setInvalidType(true);
 
   bool IsFixedPointType = DS.getTypeSpecType() == DeclSpec::TST_accum ||
                           DS.getTypeSpecType() == DeclSpec::TST_fract;
 
   // Only fixed point types can be saturated

clang/test/SemaOpenCL/access-qualifier.cl

-// RUN: %clang_cc1 -verify -pedantic -fsyntax-only -cl-std=CL1.2 %s
+// 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
 
 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

clang/test/SemaOpenCL/invalid-image.cl

 // RUN: %clang_cc1 -verify -cl-std=clc++ %s
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify %s -cl-ext=+cl_khr_gl_msaa_sharing
+// RUN: %clang_cc1 -verify %s -cl-ext=-cl_khr_gl_msaa_sharing
 // RUN: %clang_cc1 -verify -D=ATTR_TEST -fms-compatibility %s
 
 void test1(image1d_t *i) {} // expected-error-re{{pointer to type '{{__generic __read_only|__read_only}} image1d_t' is invalid in OpenCL}}
 
 void test2(image1d_t i) {
   image1d_t ti;            // expected-error{{type '__private __read_only image1d_t' can only be used as a function parameter}}
   image1d_t ai[] = {i, i}; // expected-error{{array of '__read_only image1d_t' type is invalid in OpenCL}}
   i=i; // expected-error{{invalid operands to binary expression ('__private __read_only image1d_t' and '__private __read_only image1d_t')}}
   i+1; // expected-error{{invalid operands to binary expression ('__private __read_only image1d_t' and 'int')}}
   &i; // expected-error{{invalid argument type '__private __read_only image1d_t' to unary expression}}
   *i; // expected-error{{invalid argument type '__private __read_only image1d_t' to unary expression}}
 }
 
 image1d_t test3() {} // expected-error{{declaring function return value of type '__read_only image1d_t' is not allowed}}
 
 #ifdef ATTR_TEST
 // Test case for an infinite loop bug.
 kernel void foob(read_only __ptr32  image2d_t i) { } // expected-error{{'__ptr32' attribute only applies to pointer arguments}}
 #endif
+
+typedef int image1d_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+typedef int image2d_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+typedef int image3d_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+typedef int image1d_array_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+typedef int image2d_array_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+typedef int image2d_depth_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+typedef int image1d_buffer_t; // expected-error{{cannot combine with previous 'int' declaration specifier}} expected-warning{{typedef requires a name}}
+
+// Image types from 'cl_khr_gl_msaa_sharing' are not reserved identifiers.
+typedef int image2d_msaa_t;
+typedef int image2d_array_msaa_t;
+typedef int image2d_msaa_depth_t;
+typedef int image2d_array_msaa_depth_t;
+#ifdef cl_khr_gl_msaa_sharing
+// expected-error@-5{{cannot combine with previous 'int' declaration specifier}} expected-warning@-5{{typedef requires a name}}
+// expected-error@-5{{cannot combine with previous 'int' declaration specifier}} expected-warning@-5{{typedef requires a name}}
+// expected-error@-5{{cannot combine with previous 'int' declaration specifier}} expected-warning@-5{{typedef requires a name}}
+// expected-error@-5{{cannot combine with previous 'int' declaration specifier}} expected-warning@-5{{typedef requires a name}}
+#endif
+void foo(image2d_msaa_t i);