Page MenuHomePhabricator

[OpenCL] Allow extern constant-address-space variable in non-kernel function
AbandonedPublic

Authored by yaxunl on Feb 4 2016, 11:09 AM.

Details

Summary

OpenCL v1.2 s6.8.p2 - Extern can be used for variables declared in functions.

Differential Revision: http://reviews.llvm.org/D16865

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 46929.Feb 4 2016, 11:09 AM
yaxunl retitled this revision from to [OpenCL] Allow extern constant-address-space variable in non-kernel function.
yaxunl updated this object.
yaxunl added a subscriber: tstellarAMD.
pxli168 edited edge metadata.Feb 4 2016, 8:26 PM

I read about spec and found that in OpenCL1.1 extern was not support.
The extern was supported in OpenCL1.2 and later in s6.8.p2. You can make the check to fit this and add the reference to comment.

yaxunl updated this revision to Diff 47214.Feb 8 2016, 9:05 AM
yaxunl updated this object.
yaxunl edited edge metadata.

Revised as Xiuli suggested.

Diagnostic for extern for OpenCL v1.1 and below already exists, so no change about that.
Added lit test for extern for OpenCL v1.0/v1.1.
Updated comments about OpenCL spec.

Anastasia added inline comments.Feb 9 2016, 10:06 AM
test/SemaOpenCL/extern-cl1.2.cl
1 ↗(On Diff #47214)

This is CodeGen not Sema test! Please move to CodeGenOpenCL folder.

2 ↗(On Diff #47214)

Remove this!

7 ↗(On Diff #47214)

Do we really need this function?

You can always pass -O0 to disable generated code from disappearing!

test/SemaOpenCL/extern.cl
10

Why do you need this function?

What does it add anything to testing?

yaxunl updated this revision to Diff 47346.Feb 9 2016, 11:41 AM
yaxunl marked 4 inline comments as done.

Revised as Anastasia suggested.
Moved extern test for CL1.2 from SemaOpenCL to CodeGenOpenCL.
Removed one redundant function from sema test for CL1.1.

yaxunl added inline comments.Feb 9 2016, 11:43 AM
test/SemaOpenCL/extern.cl
8

if we remove usage of foo, no code will be generated for foo even if -O0 is used. I think this is because extern foo is just declaration.

Commented at wrong location. Corrected.

test/SemaOpenCL/extern-cl1.2.cl
7 ↗(On Diff #47214)

if we remove usage of foo, no code will be generated for foo even if -O0 is used. I think this is because extern foo is just declaration.

Anastasia accepted this revision.Feb 11 2016, 10:35 AM
Anastasia edited edge metadata.

I just have a few small comments, but otherwise LGTM!

include/clang/Basic/DiagnosticSemaKinds.td
7699

. missing

7709–7710

. missing

lib/Sema/SemaDecl.cpp
6616–6619

Extern -> extern

test/SemaOpenCL/extern.cl
3

Could you please initialize the variable here to avoid the second error.

The same applies below.

8

Ok, you are right. Accepted!

This revision is now accepted and ready to land.Feb 11 2016, 10:35 AM
yaxunl updated this revision to Diff 47691.Feb 11 2016, 11:25 AM
yaxunl edited edge metadata.
yaxunl marked 3 inline comments as done.

Revised as Anastasia suggested.

Changed comments.
Added initializers in tests to avoid unnecessary error messages.

pekka.jaaskelainen edited edge metadata.

LGTM.

yaxunl marked 2 inline comments as done.Feb 17 2016, 1:17 PM

ping

pxli168 accepted this revision.Feb 17 2016, 4:41 PM
pxli168 edited edge metadata.

LGTM!
Thanks

yaxunl abandoned this revision.Mar 3 2016, 1:34 PM

this patch is superseded by D17345.