[OpenCL] Fix access qualifiers handling for typedefs

[asavonic]

Summary

OpenCL s6.6: "Access qualifier must be used with image object arguments of
kernels and of user-defined functions [...] If no qualifier is provided,
read_only is assumed".

This does not define the behaviour for image types used in typedef declaration,
but following the spec logic, we should allow access qualifiers specification
in typedefs, e.g.:

typedef write_only image1d_t img1d_wo;

Unlike cv-qualifiers, user cannot add access qualifier to the typedef type,
i.e. this is not allowed:

typedef image1d_t img1d; // note: previously declared 'read_only' here
void foo(write_only img1d im) {} // error: multiple access qualifiers

[Anastasia]

Please add cfe-commits into Subscribers.

Thanks!

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

7882 ↗

(On Diff #59491)

I am not sure why to give a warning about deduced access qualifier? It seems perfectly ok according to the spec to default to read_only if not specified.

lib/Sema/SemaType.cpp

6480 ↗

(On Diff #59491)

Not related to your change but could you fix this typo please:
can used -> can be used

test/SemaOpenCL/images-typedef.cl

7 ↗

(On Diff #59491)

Should this only be allowed for OpenCL2.0?

14 ↗

(On Diff #59491)

myRead ( -> myRead(

39 ↗

(On Diff #59491)

I feel this error might be a bit confusing, because the typedef is to an image at the end.

Did we have an error here before?

42 ↗

(On Diff #59491)

I think this is already being tested in invalid-access-qualifier.cl.

Also could we combine with that test file into one?

[asavonic]

• Cleanup images-typedef.cl
• Fix typo in SemaType.cpp
• Change diagnostic: error when access qualifier applied to a typedef

[asavonic]

• Fix images-typedef.cl for OpenCL 2.0

[asavonic]

test/SemaOpenCL/images-typedef.cl

40 ↗

(On Diff #59890)

No, but write_only qualifer was just silently ignored and img had a read_only type.

I changed the diagnostic, it should be better now:

typedef write_only image1d_t img1d_wo; // note: previously declared 'write_only' here
kernel void k7(read_only img1d_wo img){} // error: multiple access qualifiers

43 ↗

(On Diff #59890)

In this case we should rename merged test to just 'access-qualifier.cl', since images-typedef.cl contains positive test cases too. Is it ok?

[Anastasia]

lib/Sema/SemaType.cpp

6494 ↗

(On Diff #59893)

Please start the assert message from the upper case and finish with .

test/SemaOpenCL/images-typedef.cl

10 ↗

(On Diff #59893)

I think for OpenCL < 2.0 we should give an error that read_write can not be used in earlier than OpenCL version 2.0.

21 ↗

(On Diff #59893)

don't indent inside macro directives #if

41 ↗

(On Diff #59893)

Yes, looks good!

44 ↗

(On Diff #59893)

Sure, makes sense!

[asavonic]

Merged invalid-access-qualifier.cl and images-typedef.cl tests, fixed
code style issues.

[Anastasia]

LGTM! Could you please address these last minor comments though before committing.

Thanks!

test/SemaOpenCL/access-qualifier.cl

10 ↗

(On Diff #63073)

don't indent here!

68 ↗

(On Diff #63073)

It reads a bit strange. Would it be better:
"earlier than OpenCL version 2.0" -> "prior to OpenCL version 2.0"

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

2509 ↗

(On Diff #63073)

Is this change actually needed?

[asavonic]

Change diagnostic wording

[asavonic]

include/clang/Basic/DiagnosticSemaKinds.td

2509–2510 ↗

(On Diff #63222)

Yes, because I added "ExpectedParameterOrTypedef" diagnostic to support access qualifiers in typedef decl.

cfe/trunk/include/clang/Basic/Attr.td

 }
 
 // This attribute is both a type attribute, and a declaration attribute (for
 // parameter variables).
 def OpenCLAccess : Attr {
   let Spellings = [Keyword<"__read_only">, Keyword<"read_only">,
                    Keyword<"__write_only">, Keyword<"write_only">,
                    Keyword<"__read_write">, Keyword<"read_write">];
-  let Subjects = SubjectList<[ParmVar], ErrorDiag>;
+  let Subjects = SubjectList<[ParmVar, TypedefName], ErrorDiag,
+                             "ExpectedParameterOrTypedef">;
   let Accessors = [Accessor<"isReadOnly", [Keyword<"__read_only">,
                                            Keyword<"read_only">]>,
                    Accessor<"isReadWrite", [Keyword<"__read_write">,
                                             Keyword<"read_write">]>,
                    Accessor<"isWriteOnly", [Keyword<"__write_only">,
                                             Keyword<"write_only">]>];
   let Documentation = [OpenCLAccessDocs];
 }

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def err_ifunc_resolver_return : Error<
   "ifunc resolver function must return a pointer">;
 def err_ifunc_resolver_params : Error<
   "ifunc resolver function must have no parameters">;
 def warn_attribute_wrong_decl_type : Warning<
   "%0 attribute only applies to %select{functions|unions|"
   "variables and functions|functions and methods|parameters|"
   "functions, methods and blocks|functions, methods, and classes|"
   "functions, methods, and parameters|classes|enums|variables|methods|"
-  "fields and global variables|structs|variables and typedefs|thread-local variables|"
-  "variables and fields|variables, data members and tag types|"
+  "fields and global variables|structs|parameters and typedefs|variables and typedefs|"
+  "thread-local variables|variables and fields|variables, data members and tag types|"
   "types and namespaces|Objective-C interfaces|methods and properties|"
   "struct or union|struct, union or class|types|"
   "Objective-C instance methods|init methods of interface or class extension declarations|"
   "variables, functions and classes|Objective-C protocols|"
   "functions and global variables|structs, unions, and typedefs|structs and typedefs|"
   "interface or protocol declarations|kernel functions|non-K&R-style functions|"
   "variables, enums, fields and typedefs|functions, methods, enums, and classes|"
   "structs, classes, variables, functions, and inline namespaces|"
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{|earlier than OpenCL version 2.0}2">;
+  "access qualifier %0 can not be used for %1 %select{|prior to OpenCL version 2.0}2">;
 def err_opencl_multiple_access_qualifiers : Error<
   "multiple access qualifiers">;
+def note_opencl_typedef_access_qualifier : Note<
+  "previously declared '%0' here">;
 
 // OpenCL Section 6.8.g
 def err_opencl_unknown_type_specifier : Error<
   "OpenCL version %0 does not support the '%1' %select{type qualifier|storage class specifier}2">;
 
 // OpenCL v2.0 s6.12.5 Blocks restrictions
 def err_opencl_block_storage_type : Error<
   "the __block storage type is not permitted">;

cfe/trunk/include/clang/Sema/AttributeList.h

enum AttributeDeclKind {
   ExpectedFunctionMethodOrClass,
   ExpectedFunctionMethodOrParameter,
   ExpectedClass,
   ExpectedEnum,
   ExpectedVariable,
   ExpectedMethod,
   ExpectedFieldOrGlobalVar,
   ExpectedStruct,
+  ExpectedParameterOrTypedef,
   ExpectedVariableOrTypedef,
   ExpectedTLSVar,
   ExpectedVariableOrField,
   ExpectedVariableFieldOrTag,
   ExpectedTypeOrNamespace,
   ExpectedObjectiveCInterface,
   ExpectedMethodOrProperty,
   ExpectedStructOrUnion,

cfe/trunk/lib/Sema/SemaType.cpp

static void HandleNeonVectorTypeAttr(QualType& CurType,
   }
 
   CurType = S.Context.getVectorType(CurType, numElts, VecKind);
 }
 
 /// Handle OpenCL Access Qualifier Attribute.
 static void HandleOpenCLAccessAttr(QualType &CurType, const AttributeList &Attr,
                                    Sema &S) {
-  // OpenCL v2.0 s6.6 - Access qualifier can used only for image and pipe type.
+  // OpenCL v2.0 s6.6 - Access qualifier can be used only for image and pipe type.
   if (!(CurType->isImageType() || CurType->isPipeType())) {
     S.Diag(Attr.getLoc(), diag::err_opencl_invalid_access_qualifier);
     Attr.setInvalid();
     return;
   }
+
+  if (const TypedefType* TypedefTy = CurType->getAs<TypedefType>()) {
+    QualType PointeeTy = TypedefTy->desugar();
+    S.Diag(Attr.getLoc(), diag::err_opencl_multiple_access_qualifiers);
+
+    std::string PrevAccessQual;
+    switch (cast<BuiltinType>(PointeeTy.getTypePtr())->getKind()) {
+      #define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix) \
+    case BuiltinType::Id:                                          \
+      PrevAccessQual = #Access;                                    \
+      break;
+      #include "clang/Basic/OpenCLImageTypes.def"
+    default:
+      assert(0 && "Unable to find corresponding image type.");
+    }
+
+    S.Diag(TypedefTy->getDecl()->getLocStart(),
+       diag::note_opencl_typedef_access_qualifier) << PrevAccessQual;
+  }
 }
 
 static void processTypeAttrs(TypeProcessingState &state, QualType &type,
                              TypeAttrLocation TAL, AttributeList *attrs) {
   // Scan through and apply attributes to this type where it makes sense.  Some
   // attributes (such as __address_space__, __vector_size__, etc) apply to the
   // type, but others can be present in the type specifiers even though they
   // apply to the decl.  Here we apply type attributes and ignore the rest.

cfe/trunk/test/CodeGenOpenCL/kernel-arg-info.cl

 // CHECK: !kernel_arg_access_qual ![[MD42:[0-9]+]]
 // CHECK: !kernel_arg_type ![[MD43:[0-9]+]]
 // CHECK: !kernel_arg_base_type ![[MD44:[0-9]+]]
 // CHECK: !kernel_arg_type_qual ![[MD45:[0-9]+]]
 // CHECK-NOT: !kernel_arg_name
 // ARGINFO: !kernel_arg_name ![[MD46:[0-9]+]]
 
 typedef image1d_t myImage;
-kernel void foo5(read_only myImage img1, write_only image1d_t img2) {
+kernel void foo5(myImage img1, write_only image1d_t img2) {
 }
 // CHECK: define spir_kernel void @foo5{{[^!]+}}
 // CHECK: !kernel_arg_addr_space ![[MD41:[0-9]+]]
 // CHECK: !kernel_arg_access_qual ![[MD51:[0-9]+]]
 // CHECK: !kernel_arg_type ![[MD52:[0-9]+]]
 // CHECK: !kernel_arg_base_type ![[MD53:[0-9]+]]
 // CHECK: !kernel_arg_type_qual ![[MD45]]
 // CHECK-NOT: !kernel_arg_name

cfe/trunk/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=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
+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
+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}}
+#endif
+
+
+kernel void k1(img1d_wo img) {
+  myRead(img); // expected-error {{passing 'img1d_wo' (aka '__write_only image1d_t') to parameter of incompatible type '__read_only image1d_t'}}
+}
+
+kernel void k2(img1d_ro img) {
+  myWrite(img); // expected-error {{passing 'img1d_ro' (aka '__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
+kernel void k4(img1d_rw img) {
+  myReadWrite(img);
+}
+#endif
+
+kernel void k5(img1d_ro_default img) {
+  myWrite(img); // expected-error {{passing 'img1d_ro_default' (aka '__read_only image1d_t') to parameter of incompatible type '__write_only image1d_t'}}
+}
+
+kernel void k6(img1d_ro img) {
+  myRead(img);
+}
+
+kernel void k7(read_only img1d_wo img){} // expected-error {{multiple access qualifiers}}
+
+kernel void k8(write_only img1d_ro_default img){} // expected-error {{multiple access qualifiers}}
+
+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-error{{multiple access qualifiers}}
+
+#if __OPENCL_C_VERSION__ >= 200
+kernel void k13(read_write pipe int i){} // expected-error{{access qualifier 'read_write' can not be used for '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

cfe/trunk/test/SemaOpenCL/invalid-access-qualifier.cl

-// RUN: %clang_cc1 -verify %s
-// RUN: %clang_cc1 -verify -cl-std=CL2.0 -DCL20 %s
-
-void test1(read_only int i){} // expected-error{{access qualifier can only be used for pipe and image type}}
-
-void test2(read_only write_only image1d_t i){} // expected-error{{multiple access qualifiers}}
-
-void test3(read_only read_only image1d_t i){} // expected-error{{multiple access qualifiers}}
-
-#ifdef CL20
-void test4(read_write pipe int i){} // expected-error{{access qualifier 'read_write' can not be used for 'pipe int'}}
-#else
-void test4(__read_write image1d_t i) {} // expected-error{{access qualifier '__read_write' can not be used for '__read_write image1d_t' earlier than OpenCL version 2.0}}
-#endif