Page MenuHomePhabricator

[OpenCL] Apply missing restrictions for Blocks in OpenCL v2.0
ClosedPublic

Authored by Anastasia on Feb 5 2016, 9:14 AM.

Details

Reviewers
pxli168
Summary

Applying the following restrictions for block variables in OpenCL (v2.0 s6.12.5):

  • __block storage class is disallowed
  • variadic arguments are disallowed
  • every block declaration must be const qualified and initialized
  • a block can't be used as a return type of a function
  • extern speficier is disallowed

This adds diagnostics missing from http://reviews.llvm.org/D16047

Diff Detail

Event Timeline

Anastasia updated this revision to Diff 47020.Feb 5 2016, 9:14 AM
Anastasia retitled this revision from to [OpenCL] Apply missing restrictions for Blocks in OpenCL v2.0.
Anastasia updated this object.
Anastasia set the repository for this revision to rL LLVM.
Anastasia added a subscriber: cfe-commits.
Leporacanthicus added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
7684

Is this blank line there for a reason, as there seem to be none above...

7704

US spelling is initialized - which I believe is standard for Clang?

test/SemaOpenCL/invalid-blocks-cl20.cl
6

US spelling will need to be fixed here too, if fixed above.

pxli168 added inline comments.Feb 5 2016, 7:00 PM
lib/Sema/SemaDecl.cpp
13011

can -> cannot
I just find I could not understand this.

lib/Sema/SemaType.cpp
3827

OpenCL v1.2 s6.9 doesn't support variadic functions, except for ->OpenCL v1.2 s6.9 - Variadic functions are not supported, ....

Anastasia updated this revision to Diff 47823.Feb 12 2016, 11:22 AM
Anastasia removed rL LLVM as the repository for this revision.

Update according to review comments!

test/SemaOpenCL/invalid-func.cl
6 ↗(On Diff #47823)

I wonder should we check for the fingerprint too when letting printf() pass?
This is not the correct OpenCL printf() as the first argument should be in constant address space.

MatsPetersson added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
7700

Looks good

7717

Looks good now (from my official account this time! :) )

MatsPetersson added inline comments.Feb 16 2016, 6:20 AM
test/SemaOpenCL/invalid-func.cl
6 ↗(On Diff #47823)

Good spot, this should be fixed.

Anastasia updated this revision to Diff 48219.Feb 17 2016, 11:25 AM

Drafted code for printf handling.

Made me think about:

  1. How much signature check should we do i.e. should we check the pointer AS itself (generic for CL2.0, any other otherwise) or qualifiers being used for the first parameter (const, volatile ...)?
  2. Would it be nicer to just add printf as a Clang OpenCL builitin to prevent it being declared in various ways and avoid writing custom signature check as in this change?

I think adding printf as a builtin function is a good idea.

If not, I feel that the error message should distinguish between (for example):

int printf(global char* fmt, ...);

and

void myfunc(int n, ...);

since the former is "incorrect prototype of printf" and the latter is "not a valid opencl function".

@Mats, I think I would prefer not to add even more special handling (i.e. error in this case) for printf.

I will look into adding it as a Builtin instead. However, I would prefer to remove it from this change and submit a separate patch as it doesn't belong to Clang blocks anyways.

Thanks!

@Mats, I think I would prefer not to add even more special handling (i.e. error in this case) for printf.

I will look into adding it as a Builtin instead. However, I would prefer to remove it from this change and submit a separate patch as it doesn't belong to Clang blocks anyways.

Thanks!

@Anastasia: Yes, I agree, printf should be done as a separate patch.

Anastasia updated this revision to Diff 48514.Feb 19 2016, 10:38 AM

Removed varargs due to printf special handling. To be added as a separate review!

pxli168 edited edge metadata.Feb 20 2016, 6:40 PM
This comment was removed by pxli168.
pxli168 accepted this revision.Feb 20 2016, 6:41 PM
pxli168 edited edge metadata.

LGTM!
Thanks

This revision is now accepted and ready to land.Feb 20 2016, 6:41 PM
pxli168 requested changes to this revision.Feb 21 2016, 9:02 PM
pxli168 edited edge metadata.

It seems this patch will check block for CL1.2 or eailer? But the spec reference is for OpenCL v2.0.

This revision now requires changes to proceed.Feb 21 2016, 9:02 PM

Yes, that's right. In this commit I assume parser has already accepted the blocks.

We currently accept them if -fblocks is passed. I think we should also accept blocks by default with CL2.0. But I am thinking not to restrict passing -fblocks with earlier CL version. This way it's possible to enable them with earlier standards if whoever does it knows what it's needed for. We could give a warning in this case. What do you think?

However, I wouldn't block this patch on it, as I wouldn't commit them together anyways. But I am happy to prepare a separate patch.

pxli168 accepted this revision.Feb 22 2016, 5:51 PM
pxli168 edited edge metadata.

Then there will be no problem.
I will change my patches as well.
LGTM!
Thanks.

This revision is now accepted and ready to land.Feb 22 2016, 5:51 PM