This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add support of __opencl_c_program_scope_global_variables feature macro
ClosedPublic

Authored by azabaznov on May 26 2021, 11:20 AM.

Diff Detail

Event Timeline

azabaznov created this revision.May 26 2021, 11:20 AM
azabaznov requested review of this revision.May 26 2021, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 11:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
svenvh added inline comments.May 27 2021, 1:46 AM
clang/test/SemaOpenCL/storageclass.cl
71

Not sure if this is the best way to restructure this test? Now the diagnostics are quite far away from the declarations, and it is harder to see which diagnostic belongs to which declaration.

I wonder if the following would be a better structure instead:

int G3 = 0;
#ifndef __opencl_c_program_scope_global_variables
// expected-error@-2 {{program scope variable must reside in constant address space}}
#endif

global int G4 = 0;
#ifndef __opencl_c_program_scope_global_variables
// expected-error@-2 {{program scope variable must reside in constant address space}}
#endif

... etc. ...

The downside is that it is a bit more verbose due to the repetition of the #if guards, but it makes the test more readable/maintainable in my opinion.

azabaznov added inline comments.Jun 1 2021, 5:04 AM
clang/test/SemaOpenCL/storageclass.cl
71

Yeah, that's reasonable. I'll change that in the following patch. Thanks.

azabaznov updated this revision to Diff 352723.Jun 17 2021, 7:44 AM

Restructured test, added comments

azabaznov marked an inline comment as done.Jun 17 2021, 7:50 AM
Anastasia accepted this revision.Jun 22 2021, 7:59 AM

LGTM! Thanks!

clang/include/clang/Basic/OpenCLOptions.h
83

While it is not critical, I would slightly prefer that we are checking for == 300. Checking >= 300 assumes that the next versions are going to be backward compatible. But it is not always the case if we look at the OpenCL evolution.

This revision is now accepted and ready to land.Jun 22 2021, 7:59 AM
azabaznov updated this revision to Diff 355884.Jul 1 2021, 8:02 AM

Fixed latest comment about checking for == 300. Btw, there already exists a few places where >= 300 is checked. Should fix them as well.

azabaznov marked an inline comment as done.Jul 1 2021, 8:03 AM
azabaznov updated this revision to Diff 358236.Jul 13 2021, 5:06 AM

Update test after generic AS was merged

azabaznov updated this revision to Diff 358909.Jul 15 2021, 3:46 AM

Restructured test a little

This revision was landed with ongoing or failed builds.Jul 15 2021, 7:21 AM
This revision was automatically updated to reflect the committed changes.