This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Fix access qualifiers handling for typedefs
ClosedPublic

Authored by asavonic on Jun 2 2016, 11:34 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

asavonic updated this revision to Diff 59491.Jun 2 2016, 11:34 PM
asavonic retitled this revision from to [OpenCL] Fix access qualifiers handling for typedefs.
asavonic updated this object.
asavonic added a reviewer: Anastasia.
Anastasia edited edge metadata.Jun 3 2016, 10:51 AM

Please add cfe-commits into Subscribers.

Thanks!

asavonic edited edge metadata.Jun 6 2016, 1:37 AM
asavonic added a subscriber: cfe-commits.
Anastasia added inline comments.Jun 6 2016, 9:11 AM
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 updated this revision to Diff 59890.Jun 7 2016, 7:49 AM
  • Cleanup images-typedef.cl
  • Fix typo in SemaType.cpp
  • Change diagnostic: error when access qualifier applied to a typedef
asavonic updated this object.Jun 7 2016, 7:52 AM
asavonic updated this revision to Diff 59893.Jun 7 2016, 8:21 AM
asavonic marked 3 inline comments as done.
  • Fix images-typedef.cl for OpenCL 2.0
asavonic added inline comments.Jun 7 2016, 8:33 AM
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 added inline comments.Jun 8 2016, 10:07 AM
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 updated this revision to Diff 63073.Jul 7 2016, 7:44 AM

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

asavonic marked 4 inline comments as done.Jul 7 2016, 7:51 AM
Anastasia accepted this revision.Jul 7 2016, 9:24 AM
Anastasia edited edge metadata.

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"

This revision is now accepted and ready to land.Jul 7 2016, 9:24 AM
Anastasia added inline comments.Jul 7 2016, 9:30 AM
include/clang/Basic/DiagnosticSemaKinds.td
2509 ↗(On Diff #63073)

Is this change actually needed?

asavonic updated this revision to Diff 63222.Jul 8 2016, 7:32 AM
asavonic edited edge metadata.

Change diagnostic wording

asavonic marked 2 inline comments as done.Jul 8 2016, 7:34 AM
asavonic marked an inline comment as done.Jul 8 2016, 7:38 AM
asavonic added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
2509–2510 ↗(On Diff #63222)

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

This revision was automatically updated to reflect the committed changes.
cfe/trunk/test/SemaOpenCL/access-qualifier.cl