Page MenuHomePhabricator

[OpenCL] Enable program scope variables for OpenCL2.0
ClosedPublic

Authored by Anastasia on Sep 23 2015, 9:34 AM.

Details

Summary

I would like to commit this change that enables program scope variables (PSVs) – an OpenCL2.0 feature (Section 6.5.1).

There are three major changes in this patch.

First change removes virtual SC_OpenCLWorkGroupLocal storage type specifier as it conflicts with static
local variables now and prevents diagnosing static local address space variables correctly. Also it doesn’t seem really
necessary since we can identify local AS variable by looking at the AS attribute itself.

Second change makes static local variables as well as global variables allowed in OpenCL2.0 (OpenCL2.0 s6.8 and s6.5.1).

Last change improves diagnostics of allowed ASes for variables in different scopes:

Global or static local variables have to be in global or constant ASes (OpenCL1.2 s6.5, OpenCL2.0 s6.5.1);
Non-kernel function variables can’t be declared in local or constant ASes (OpenCL1.1 s6.5.2 and s6.5.3).

Diff Detail

Event Timeline

Anastasia updated this revision to Diff 35520.Sep 23 2015, 9:34 AM
Anastasia retitled this revision from to [OpenCL] Enable program scope variables for OpenCL2.0.
Anastasia updated this object.
Anastasia added a reviewer: pekka.jaaskelainen.
Anastasia added a subscriber: cfe-commits.
pekka.jaaskelainen requested changes to this revision.Sep 24 2015, 10:12 AM
pekka.jaaskelainen edited edge metadata.

Could only find style nitpicks in this one.

include/clang/Basic/DiagnosticSemaKinds.td
7473

variable

lib/Sema/SemaDecl.cpp
6444

There are white space issues in this patch block.

6470

Is there really no simpler way to find the FunctionDecl?

6473

Is there a reason not to use the ! operator instead of comparing to false?

This revision now requires changes to proceed.Sep 24 2015, 10:12 AM
bader added a subscriber: bader.Sep 24 2015, 1:23 PM
bader added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
7472–7473

I think we can do better diagnostics. Original message hinted on way to fix that error - add 'constant' qualifier.

In our implementation we have 4 kinds of diagnostics (probably we need only 2 of them):
def err_opencl_global_invalid_addr_space : Error<
"program scope variables must reside in constant address space">;
def err_opencl20_global_invalid_addr_space : Error<
"program scope variables must reside in global or constant address space">;
def err_program_scope_variable_non_constant : Error<
"program scope variables are required to be declared in constant address space">;
def err_program_scope_variable_non_constant_or_global : Error<
"program scope variables are required to be declared either in constant or global address space">;

They used for OpenCL 1.2 and OpenCL 2.0 sources correspondingly.

test/SemaOpenCL/storageclass.cl
8–10

Don't you think it's better to put OpenCL 2.0 test into separate file?

Anastasia updated this revision to Diff 35732.Sep 25 2015, 10:15 AM
Anastasia edited edge metadata.

Thanks! The review comments are addressed in this update!

pekka.jaaskelainen edited edge metadata.
This revision is now accepted and ready to land.Sep 27 2015, 11:18 AM
Anastasia closed this revision.Jul 12 2016, 10:11 AM

r248906 and r250892