Page MenuHomePhabricator

[OpenCL] Setting constant address space for array initializers

Authored by AlexeySotkin on Oct 6 2016, 12:54 AM.

Event Timeline

AlexeySotkin retitled this revision from to [OpenCL] Setting constant address space for array initializers.
AlexeySotkin updated this object.
AlexeySotkin added reviewers: Anastasia, yaxunl, bader.
AlexeySotkin set the repository for this revision to rL LLVM.
AlexeySotkin added a subscriber: AlexeySotkin.
yaxunl edited edge metadata.Oct 6 2016, 7:31 AM

Please add a test. Thanks.

AlexeySotkin edited edge metadata.
AlexeySotkin removed rL LLVM as the repository for this revision.

Adding test checking address space of array initializer

The test looks OK. However, the patch only contains the test now.

Squashing commits

Now there should be both :)

yaxunl accepted this revision.Oct 14 2016, 8:46 AM
yaxunl edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 14 2016, 8:46 AM
bader accepted this revision.Oct 19 2016, 3:06 AM
bader edited edge metadata.


Anastasia added inline comments.Oct 19 2016, 10:34 AM

Why this change?

AlexeySotkin added inline comments.Oct 24 2016, 1:51 AM

Without this change, global variables with unnamed address space are translated to SPIR-V as variables with "Function" storage class, which is wrong.
This should fix this issue:

bader added inline comments.Oct 24 2016, 3:37 AM

There is inconsistency with how Clang maps initializers to OpenCL memory model.
Consider example from the test case:

__private int arr[] = {1, 2, 3};

This code allocates a global constant for initializer {1, 2, 3} and later copies values from this global constant to the private array using memcpy intrinsic. The global constant must be allocated in constant address space, but old code do assign any address space, which is considered to be a private memory region.
This patch puts global constant objects to constant address space.

Anastasia added inline comments.Oct 24 2016, 10:16 AM

Yes, this is perfectly fine. I was specifically asking about setting llvm::GlobalValue::UnnamedAddr::None attribute. I can't see why this has to be done or is it because we now assign concrete address space and private was treated as no address space at all?

bader added inline comments.Oct 25 2016, 3:25 AM

This attribute has nothing to do with address spaces.
See for local_unnamed_addr and unnamed_addr attributes description.
My understanding is that llvm::GlobalValue::UnnamedAddr::Global should be fine here.

AlexeySotkin added inline comments.Oct 25 2016, 7:48 AM

llvm::GlobalValue::UnnamedAddr::None is default value of UnnamedAddr for GlobalValue. This line can be removed, but extra "if" statement must be added before GV->setUnnamedAddr(UA);

Anastasia added inline comments.Oct 25 2016, 10:19 AM

Yes, I also think that leaving global should be fine here. So could we just undo the change to line 1274?

AlexeySotkin edited edge metadata.

Setting UnnamedAddr to Global

AlexeySotkin added inline comments.Oct 26 2016, 8:02 AM

Changed to Global

Anastasia accepted this revision.Oct 26 2016, 10:15 AM
Anastasia edited edge metadata.

LGTM! Thanks!

AlexeySotkin closed this revision.Oct 27 2016, 11:20 PM
bader added a comment.Oct 31 2016, 3:36 AM

Committed at 285557 with updated tests.