Page MenuHomePhabricator

[OpenCL] Add Sema checks for OpenCL 2.0 block
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

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

opencl -> OpenCL

7723 ↗(On Diff #48454)

opencl -> OpenCL

lib/Sema/SemaDecl.cpp
6712 ↗(On Diff #48454)

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

6714 ↗(On Diff #48454)

getLangOpts() -> LangOpts

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

6717 ↗(On Diff #48454)

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

lib/Sema/SemaExpr.cpp
6520 ↗(On Diff #48454)

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

6523 ↗(On Diff #48454)

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

6525 ↗(On Diff #48454)

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

10358 ↗(On Diff #48454)

Block -> block

10359 ↗(On Diff #48454)

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

10411 ↗(On Diff #48454)

Block -> block

lib/Sema/SemaType.cpp
2178 ↗(On Diff #48454)

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

test/SemaOpenCL/invalid-block.cl
11 ↗(On Diff #48454)

array of block type ...

12 ↗(On Diff #48454)

block type cannot ...

13 ↗(On Diff #48454)

block type cannot ...

16 ↗(On Diff #48454)

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

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

lib/Sema/SemaExpr.cpp
6525 ↗(On Diff #48454)

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

test/SemaOpenCL/invalid-block.cl
16 ↗(On Diff #48454)

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.

Anastasia added inline comments.Feb 22 2016, 9:55 AM
lib/Sema/SemaDecl.cpp
6714 ↗(On Diff #48648)

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

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

pxli168 added inline comments.
lib/Sema/SemaDecl.cpp
6714 ↗(On Diff #48648)

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

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 ↗(On Diff #48774)
This revision was automatically updated to reflect the committed changes.