This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Refine OpenCLImageAccessAttr to OpenCLAccessAttr
ClosedPublic

Authored by pxli168 on Jan 10 2016, 3:21 AM.

Details

Summary

OpenCL access qualifiers are now not only used for image types, refine it to avoid misleading,

Add semacheck for OpenCL access qualifier as well as test caees.

Diff Detail

Event Timeline

pxli168 updated this revision to Diff 44426.Jan 10 2016, 3:21 AM
pxli168 retitled this revision from to [OpenCL] Refine OpenCLImageAccessAttr to OpenCLAccessAttr.
pxli168 updated this object.
pxli168 added a subscriber: cfe-commits.
Anastasia added inline comments.Jan 11 2016, 8:10 AM
lib/Sema/SemaDeclAttr.cpp
4934

In the case of failure, where would destructor of AccessAttr be called?

Could we invoke it before exiting? Or alternatively it's possible to check an access kind based on string matching from Attr getName() and create AccessAttr later on success.

5694

So where would the attribute check happen? I don't think the comment is very clear.

lib/Sema/SemaType.cpp
6223

remove one -

test/SemaOpenCL/invalid-kernel-attrs.cl
31

Strangely, I don't see this diagnostic being added in this patch.

pxli168 added inline comments.Jan 11 2016, 8:54 PM
lib/Sema/SemaDeclAttr.cpp
4934

Nice catch, I think I will try if getName() can handle this, or I think maybe I need a destructor.

5694

Maybe I don't make my self claer.

The pipe decl like

SomeFn(read_only pipe int p)

It will process read_only twice which is not expected, this will make attr add twice.

I will make the comment clearer to understand.

test/SemaOpenCL/invalid-kernel-attrs.cl
31

As you can see, the read_only used to use with int. Now we check the type, so int could not used for this test more. Just change the in to image1d_t to keep the old test case work.

pxli168 updated this revision to Diff 44600.Jan 11 2016, 8:56 PM

Fix destruct problem and fix style in inline comment.

Anastasia edited edge metadata.Jan 18 2016, 6:02 AM

I think some tests for new diagnostics are still missing?

include/clang/Basic/DiagnosticSemaKinds.td
7676

before CL2.0 -> earlier than OpenCL2.0 versions

lib/Sema/SemaDeclAttr.cpp
4922

could we write it shorter somehow:

read_write can be used for image types to specify that an image object can be read and written.

5692

Ah, you mean the attr was already process with element type?

I don't think this comment is clear still.

Could you write something like:
Skip if pipe type as already being processes with an element type.

5696

Could you also avoid using break. I think it should be quite easy here.

test/SemaOpenCL/invalid-kernel-attrs.cl
31–32

LG!

pxli168 updated this revision to Diff 45681.Jan 22 2016, 5:16 AM
pxli168 edited edge metadata.
pxli168 marked 4 inline comments as done.

Rewrite some comments and add missing invalid test case.

Anastasia added inline comments.Jan 26 2016, 4:30 AM
lib/Sema/SemaDeclAttr.cpp
5705

Do you mean to remove "twice" here? As I think that's what you are trying to avoid.

test/Parser/opencl-image-access.cl
2

please, add -verify flag

10

Btw, I can see that read_write is now accepted even if -cl-std=CL1.1. So essentially you don't need to add CL20 checks here.

But I might prepare a quick fix of it afterwards.

test/SemaOpenCL/invalid-access-qualifier.cl
12

this is being tested in test/Parser/opencl-image-access.cl already

pxli168 added inline comments.Jan 26 2016, 9:04 PM
test/Parser/opencl-image-access.cl
10

Yes, this is what this patch added in. It will check if read_write used earlier than CL2.0 as the spec said.

test/SemaOpenCL/invalid-access-qualifier.cl
12

I will remove it.

pxli168 updated this revision to Diff 46865.Feb 3 2016, 8:46 PM

Remove repeat test case.

pxli168 added inline comments.Feb 3 2016, 8:50 PM
lib/Sema/SemaDeclAttr.cpp
5068

I used arc to update the patch and it case some line error.
I have check about OpenCL version here and will handle the read_write with image before OpenCL 2.0. As spec mentioned that it is reversed before OpenCL 2.0.

Anastasia added inline comments.Feb 5 2016, 8:17 AM
lib/Sema/SemaDeclAttr.cpp
5068

Do you think we could just check:

if (S.getLangOpts().OpenCLVersion < 200 || DeclTy->isPipeType())
pxli168 updated this revision to Diff 47073.Feb 5 2016, 6:49 PM

Make some optimization

lib/Sema/SemaDeclAttr.cpp
5068

It seems can work, but maybe mislead to others.
But with the comment above it seems ok.

Adding a few final comments, otherwise, looks good!

lib/Sema/SemaDeclAttr.cpp
5051

there is only one

5059

OpenCL v2.0 s6.6: -> OpenCL v2.0 s6.6 -

5061

OpenCL v2.0 s6.13.6: -> OpenCL v2.0 s6.13.6 -

5816

Did you mean to say:

"Skip pipe type, otherwise it will be processed twice with its element type"

lib/Sema/SemaType.cpp
6243

OpenCL v2.0 s6.6: -> OpenCL v2.0 s6.6 -

Qualifier -> qualifier

test/Parser/opencl-image-access.cl
2

ping

pxli168 updated this revision to Diff 47455.Feb 10 2016, 8:00 AM
pxli168 marked 5 inline comments as done.
Anastasia accepted this revision.Feb 11 2016, 10:53 AM
Anastasia edited edge metadata.

LGTM!

@pekka, any comments here? We will finalize it if not.

lib/Sema/SemaDeclAttr.cpp
5816

. missing

This revision is now accepted and ready to land.Feb 11 2016, 10:53 AM
pekka.jaaskelainen edited edge metadata.

Otherwise LGTM.

lib/Sema/SemaChecking.cpp
267–268

This TODO can be removed?

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
include/clang/Basic/Attr.td
678

Please, if you are updating this attribute, document it.

lib/Sema/SemaDeclAttr.cpp
5051

Missing punctuation. ;-)

5053

This should be pointing to the attribute, not the declaration, shouldn't it?

5064

Drop the llvm namespace specifier.

5068

This seems like it should also be pointing to the attribute instead of the declaration (I assume the declaration would be fine were it not for the presence of the attribute).

5816

Missing punctuation.

Of bigger concern -- this code is highly suspect -- this is called for every declaration in the source code. Why is this complexity needed here instead of elsewhere?

Anastasia added inline comments.Feb 12 2016, 10:19 AM
lib/Sema/SemaDeclAttr.cpp
5816

I assume we could do this check in ProcessDeclAttributeList or just after:

if (const AttributeList *Attrs = PD.getTypeObject(i).getAttrs())
aaron.ballman added inline comments.Feb 12 2016, 10:29 AM
lib/Sema/SemaDeclAttr.cpp
5816

Is this an attribute that's written on the type declarator, and needs to be "slid" onto the declaration? If so, we do that in ProcessDeclAttributes, around line 5794. But we should not be adding ad hoc special cases to filter these out, they should probably be handled in distributeTypeAttrsFromDeclarator() in SemaType.cpp.

pxli168 updated this revision to Diff 48046.Feb 15 2016, 11:30 PM
pxli168 edited edge metadata.
pxli168 marked 3 inline comments as done.

Update doc for OpenCL access qualifier, still find where to handle access qualifier for pipe type.

pxli168 requested a review of this revision.Feb 15 2016, 11:48 PM
pxli168 edited edge metadata.

Need to refine access qualifier with pipe type.

lib/Sema/SemaDeclAttr.cpp
5053

In this case there will be two attributes, so I think it should just point to the declaration.

5816–5817

I think this attribute one that need to be slid on the declaration. Now I just handle this in ProcessDeclAttributes, with

const ParmVarDecl *PDecl = llvm::dyn_cast<ParmVarDecl>(D);
if (!PDecl ||
    !PDecl->getType().getCanonicalType().getTypePtr()->isPipeType()) {

And I tried to handle it in distributeTypeAttrsFromDeclarator but it seems this attribute does not go through that path and I am still working on how to place this check.

aaron.ballman added inline comments.Feb 16 2016, 6:56 AM
lib/Sema/SemaDeclAttr.cpp
5053

That's why I think it should be pointing at the attribute. The presence of the second attribute is what triggers the diagnostic, not anything about the declaration. The location should always point to "this thing is what caused the diagnostic."

5069

Spurious TODO?

5816–5817

You should look into why it does not make its way into distributeTypeAttrsFromDeclarator(); as it stands, I am uncomfortable with the current approach.

Hi Anastasia,
It seems the access qualifier attribute may need to be handled like the you have mentioned in clang-commit that to be in type.
What is you opinion? I can write a patch for that too.

Thanks

lib/Sema/SemaDeclAttr.cpp
5816–5817

Hi Aaron,
It seems that OpenCL type that use this attribute did not go through this path, and I will find a way to handle this.

Thanks

pxli168 planned changes to this revision.Feb 18 2016, 1:23 AM

It seems it is related with the pipe type. I am still working on how to fix this problem.

pxli168 updated this revision to Diff 48452.Feb 18 2016, 8:28 PM
pxli168 edited edge metadata.

Refine the pipe parse to solve the problem the attribute for pipe will be handled twice within Declarator

Anastasia added inline comments.Feb 19 2016, 11:03 AM
lib/Sema/SemaDeclAttr.cpp
5053

Yes, I think attribute would make more sense.

5068

Yes, w/o this attribute it would assume default access qualifier i.e. read_only. So I agree to point to the attribute here too.

pxli168 added inline comments.Feb 21 2016, 10:12 PM
lib/Sema/SemaDeclAttr.cpp
5053

Already changed to attribute.

5068

Already done.

A few small comments!

include/clang/Basic/AttrDocs.td
1578

what about __read_write?

include/clang/Basic/DiagnosticSemaKinds.td
7742

could you write __read_write/read_write please!

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
24

This test doesn't seem to work correctly because it didn't detect that we were checking for the wrong error message!

I am thinking it might be the issue of adding "-ferror-limit 100" to the RUN line as it might have stopped to diagnose after certain number of errors given. Could you please double check this?

Anastasia added inline comments.Feb 23 2016, 4:26 AM
test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
24

I just checked it and I guess it's passing because it's doing substring check, so no need to add anything to the RUN line. Your change is fine here!

aaron.ballman edited edge metadata.Feb 23 2016, 7:28 AM

Mostly minor comments, but I like this approach!

include/clang/Basic/DiagnosticSemaKinds.td
7742

Better yet, use the spelling the user wrote, since you have access to that information already.

lib/Sema/SemaType.cpp
6240

Missing a period at the end of the sentence.

6243

Missing a period at the end of the sentence.

Anastasia added inline comments.Feb 24 2016, 9:52 AM
include/clang/Basic/AttrDocs.td
1578

Oh, I see it now. May be we could group

__read_only/read_only, __write_only/write_only ...
pxli168 marked 2 inline comments as done.Feb 24 2016, 9:21 PM

Remove test case for access quilifier in test/SemaOpenCL/invalid-kernel-attrs.cl.
Due to the patch http://reviews.llvm.org/D17437.
read_only can only be used in parameters with pipe and image type.

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
24

I found these bug by testing with the access qualifier, and find some word missing and just add them in this patch.

pxli168 updated this revision to Diff 49007.Feb 24 2016, 9:28 PM
pxli168 edited edge metadata.
Anastasia accepted this revision.Feb 25 2016, 4:18 AM
Anastasia edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Feb 25 2016, 4:18 AM
aaron.ballman accepted this revision.Feb 25 2016, 5:46 AM
aaron.ballman edited edge metadata.

LGTM, thanks!

pxli168 closed this revision.Feb 25 2016, 7:17 PM