Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 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
5067

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
5067

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
5067

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
5050

there is only one

5058

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

5060

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

5823–5835

Did you mean to say:

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

lib/Sema/SemaType.cpp
6231

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

Qualifier -> qualifier

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

ping

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
5823

. 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

This TODO can be removed?

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

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

lib/Sema/SemaDeclAttr.cpp
5050

Missing punctuation. ;-)

5052

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

5063

Drop the llvm namespace specifier.

5067

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).

5823

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
5823

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
5823

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 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
5052

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

5823–5836

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
5052

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."

5068

Spurious TODO?

5823–5836

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
5823–5836

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 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
5052

Yes, I think attribute would make more sense.

5067

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
5052

Already changed to attribute.

5067

Already done.

A few small comments!

include/clang/Basic/AttrDocs.td
1572 ↗(On Diff #48452)

what about __read_write?

include/clang/Basic/DiagnosticSemaKinds.td
7713

could you write __read_write/read_write please!

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
24 ↗(On Diff #48452)

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 ↗(On Diff #48452)

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
7713

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

lib/Sema/SemaType.cpp
6228

Missing a period at the end of the sentence.

6231

Missing a period at the end of the sentence.

Anastasia added inline comments.Feb 24 2016, 9:52 AM
include/clang/Basic/AttrDocs.td
1572 ↗(On Diff #48452)

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 ↗(On Diff #48452)

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

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