This is an archive of the discontinued LLVM Phabricator instance.

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

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

Download Raw Diff

Details

Reviewers

Anastasia
pekka.jaaskelainen
pxli168

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 reviewers: Anastasia, pxli168, pekka.jaaskelainen.

yaxunl added a subscriber: • tstellarAMD.

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.

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
12	Why do you need this function? What does it add anything to testing?

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.

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

include/clang/Basic/DiagnosticSemaKinds.td
7699 ↗	(On Diff #47346)	. missing
7709 ↗	(On Diff #47346)	. missing
lib/Sema/SemaDecl.cpp
6616	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

Revised as Anastasia suggested.

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

LGTM.

ping

LGTM!
Thanks

this patch is superseded by D17345.

Revision Contents

Path

Size

lib/

Sema/

SemaDecl.cpp

3 lines

test/

SemaOpenCL/

extern.cl

7 lines

Diff 46929

lib/Sema/SemaDecl.cpp

Context not available.
	}	}
	// OpenCL v1.1 s6.5.2 and s6.5.3 no local or constant variables	// OpenCL v1.1 s6.5.2 and s6.5.3 no local or constant variables
	// in functions.	// in functions.
	if (T.getAddressSpace() == LangAS::opencl_constant \|\|	if ((T.getAddressSpace() == LangAS::opencl_constant &&
		!NewVD->isLocalExternDecl()) \|\|
	T.getAddressSpace() == LangAS::opencl_local) {	T.getAddressSpace() == LangAS::opencl_local) {
	FunctionDecl *FD = getCurFunctionDecl();	FunctionDecl *FD = getCurFunctionDecl();
	if (FD && !FD->hasAttr<OpenCLKernelAttr>()) {	if (FD && !FD->hasAttr<OpenCLKernelAttr>()) {
Context not available.
		AnastasiaUnsubmitted Done Reply Inline Actions Extern -> extern Anastasia: Extern -> extern

test/SemaOpenCL/extern.cl

Context not available.
	kernel void test(global float* buf) {	kernel void test(global float* buf) {
	buf[0] += foo;	buf[0] += foo;
		yaxunlAuthorUnsubmitted Not Done Reply Inline Actions 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. yaxunl: if we remove usage of foo, no code will be generated for foo even if -O0 is used. I think this…
		AnastasiaUnsubmitted Done Reply Inline Actions Ok, you are right. Accepted! Anastasia: Ok, you are right. Accepted!
	}	}

		void test2(global float* buf) {
		// CHECK: @bar = external addrspace(3) constant [0 x float]
		AnastasiaUnsubmitted Done Reply Inline Actions Why do you need this function? What does it add anything to testing? Anastasia: Why do you need this function? What does it add anything to testing?
		extern constant float bar[];
		buf[0] += bar[0];
		}

Context not available.