This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add Sema checks for OpenCL 2.0 block
ClosedPublic

Authored by pxli168 on Feb 18 2016, 11:53 PM.

Diff Detail

Event Timeline

pxli168 updated this revision to Diff 48454.Feb 18 2016, 11:53 PM
pxli168 retitled this revision from to [OpenCL] Add Sema checks for OpenCL 2.0 block.
pxli168 updated this object.
pxli168 updated this object.
pxli168 added a subscriber: cfe-commits.
Anastasia added inline comments.Feb 19 2016, 11:28 AM
include/clang/Basic/DiagnosticSemaKinds.td
7701

opencl -> OpenCL

7705

opencl -> OpenCL

lib/Sema/SemaDecl.cpp
6712

Could you write it a bit shorter, may be: Blocks with variadic arguments are not allowed.

6714

getLangOpts() -> LangOpts

Could you remove 'LangOpts.OpenCLVersion >= 200' because we can also enable Blocks with -fblocks flag.

6717

This seems redundant with T->isBlockPointerType() from above

lib/Sema/SemaExpr.cpp
6504

Can you shorten this comment too. I would just remove: "To support these behaviors, additional restrictions in addition to the above feature restrictions are:".

6507

Also could you remove 'getLangOpts().OpenCLVersion >= 200', see similar comment above.

6509

I suggest to unify with another check for OpenCL on line 6497 to have OpenCL bits all in one place.

10261

Block -> block

10262

Could you remove 'getLangOpts().OpenCLVersion >= 200' here too!

10311

Block -> block

lib/Sema/SemaType.cpp
2178

Could you change to "Arrays of blocks are not supported.".

test/SemaOpenCL/invalid-block.cl
12

array of block type ...

13

block type cannot ...

14

block type cannot ...

17

Actually I think pointers to blocks have to be disallowed too, but spec doesn't mention that although it forbids the dereferencing.

I will submit a bug to Khronos to clarify this.

pxli168 marked 11 inline comments as done.Feb 21 2016, 10:09 PM

Block is an OpenCL v2.0 feature, I think all test should be handled only for CL2.0 or newer version.

lib/Sema/SemaDecl.cpp
6714

But this is a OpenCL v2.0 restriction, if we do not check about OpenCL version it seems meaningless.

lib/Sema/SemaExpr.cpp
6509

It seems move it to there may change the order of checking condition first.

test/SemaOpenCL/invalid-block.cl
17

I think it is reasonable, please add me in cc list. I will try to make a change here. But this may make deference test case hard to write.

BTW, the qualifier to workgroup pipe builtin functions is resolved by https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15541.

pxli168 updated this revision to Diff 48648.Feb 21 2016, 10:09 PM
Anastasia added inline comments.Feb 22 2016, 9:55 AM
lib/Sema/SemaDecl.cpp
6714

Yes, but you have to diagnose blocks correctly in all CL versions in which we accept blocks. When you come to this point here you should only care about checking whether blocks have variadic prototype or not. You should assume the parser did the right job to either accept or reject the block declaration earlier and also gave the right diagnostic.

It will be responsibility of a parser to correctly accept or reject blocks depending on a version. I will prepare a patch for it later on. Please see my related comment on the review: http://reviews.llvm.org/D16928

lib/Sema/SemaExpr.cpp
6509

What is the implication of that? Will we get errors in different order then?

pxli168 updated this revision to Diff 48774.Feb 22 2016, 8:57 PM
pxli168 added inline comments.
lib/Sema/SemaDecl.cpp
6714

I will change these, if we can restrict -fblock with -cl-std=CL2.x then there will be no problem by only checking LangOpts.OpenCL

lib/Sema/SemaExpr.cpp
6509

I think we should put operands check after condition check to keep the original logic.

Anastasia accepted this revision.Feb 23 2016, 7:14 AM
Anastasia edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Feb 23 2016, 7:14 AM
Anastasia added inline comments.Feb 23 2016, 10:43 AM
test/SemaOpenCL/invalid-block.cl
17
This revision was automatically updated to reflect the committed changes.