This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Improve diagnostics of address spaces for variables inside function
ClosedPublic

Authored by Anastasia on Feb 17 2016, 11:54 AM.

Details

Reviewers
yaxunl
Summary

This patch:

  1. Prevents local variables to be declared in global AS
  2. Diagnoses AS of local variables with an extern storage class as if they would be in a program scope

Diff Detail

Event Timeline

Anastasia updated this revision to Diff 48225.Feb 17 2016, 11:54 AM
Anastasia retitled this revision from to [OpenCL] Improve diagnostics of address spaces for variables inside function.
Anastasia updated this object.
Anastasia added a subscriber: cfe-commits.
mgrang added a subscriber: mgrang.Feb 17 2016, 11:58 AM
mgrang added inline comments.
test/SemaOpenCL/storageclass-cl20.cl
7

Pointers should align to the right. Like:

global int *GP7 = 0;

yaxunl added inline comments.Feb 17 2016, 1:11 PM
include/clang/Basic/DiagnosticSemaKinds.td
7670

is it better to use 'function scope' instead of 'non-program scope' ?

test/SemaOpenCL/storageclass-cl20.cl
1

remove -DCL20

18

'extern int G6' refers to a program-scope variable 'int G6', since the default addr space of program-scope var is global, shouldn't 'extern int G6' have global addr space by default instead of private addr space?

Anastasia updated this revision to Diff 48516.Feb 19 2016, 10:47 AM

Fixed error msg and test!

Anastasia updated this revision to Diff 48518.Feb 19 2016, 10:49 AM

Corrected to new error msg in tests too!

Anastasia added inline comments.Feb 19 2016, 11:49 AM
test/SemaOpenCL/storageclass-cl20.cl
23

Apparently not, because we only apply global as default address space to program (non-function) scope declarations. I am not sure what we should do here though, but this perhaps doesn't belong to this change.

Do we need clarifications with Khronos here?

yaxunl accepted this revision.Feb 19 2016, 12:03 PM
yaxunl edited edge metadata.

LGTM. Thanks.

test/SemaOpenCL/storageclass-cl20.cl
23

Agree this may need another patch.

The spec is not clear about default addr space for function-scope extern variables. I think better to ask Khronos for clarification.

This revision is now accepted and ready to land.Feb 19 2016, 12:03 PM
This comment was removed by Anastasia.
Anastasia updated this revision to Diff 48826.Feb 23 2016, 9:30 AM
Anastasia removed a reviewer: pekka.jaaskelainen.
Anastasia added a subscriber: pekka.jaaskelainen.

I am adding a small clean up here for duplicate code!

@sam, could you please re-check again. Thanks!

yaxunl added inline comments.Feb 23 2016, 9:52 AM
test/SemaOpenCL/storageclass-cl20.cl
11

this error msg is confusing. better say 'function scope variable with static storage must ...'

Anastasia added inline comments.Feb 23 2016, 10:27 AM
test/SemaOpenCL/storageclass-cl20.cl
19
Anastasia added inline comments.Feb 24 2016, 9:56 AM
test/SemaOpenCL/storageclass-cl20.cl
11

Yes, I agree. The error message is not ideal. I will think of better wording here.

Anastasia updated this revision to Diff 49651.Mar 2 2016, 11:14 AM

Improved message text!