[SYCL] Change adress space for global variables #2534

fadeeval · 2020-09-24T10:20:53Z

GlobalVariables shouldn't have private address space.
This PR change usage of private address space to global for global variables.
Private address space maps to Function StorageClass in llvm-spirv translator, but global declarations shouldn't have Function Storage Class due to SPIRV spec (https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html).

Signed-off-by: Aleksander Fadeev [email protected]

Signed-off-by: Aleksander Fadeev <[email protected]>

premanandrao · 2020-09-24T14:19:16Z

Looks like you are only changing the address space for string literals. Is that correct? PR title seems to be more general.

fadeeval · 2020-09-24T14:24:53Z

The getStringLiteralAddressSpace() function is not being used for string literal only.

fadeeval · 2020-09-24T14:28:10Z

But maybe I didn't write description exactly correct, now it should be more understandable.

AlexeySotkin · 2020-09-24T14:35:21Z

clang/lib/CodeGen/CodeGenModule.cpp

@@ -4012,7 +4012,7 @@ LangAS CodeGenModule::getStringLiteralAddressSpace() const {
    //   const char *getLiteral() n{
    //     return "AB";
    //   }
-    return LangAS::opencl_private;
+    return LangAS::opencl_global;


To me, opencl_constant looks like a right value to return. Could you explain why we use opencl_global here?

There are too many constraints on OpenCL constant address space: limited memory capacity, pointers that cannot be cast to generic address space...
So even it seems like the obvious answer, it causes a lot of trouble.
But since you are asking: there is obviously a missing comment explaining the motivation in the code... :-(
Actually any question in a PR means at least 1 lacking comment in the code. ;-)

In this case though, there is a comment just above the differences that says:
```
// If we keep a literal string in constant address space, the following code
// becomes illegal:
//
// const char *getLiteral() n{
// return "AB";
// }

Yes but it does not explains why it becomes illegal.
The more comments, the happier I am. :-)

There is a addressspacecast to generic address space in IR, but adressspacecast from constant to generic forbitten because of constant address space is not part of generic address space. @keryell, should I write it in source code comments?

Yes please, add a lot of comments useful to understand the big picture. Thanks.

elizabethandrews · 2020-09-24T15:38:44Z

The getStringLiteralAddressSpace() function is not being used for string literal only.

Can you add tests for all the cases you're trying to support? The current test modifications only show address space change for string literals.

I took a quick look at the code to see where getStringLiteralAddressSpace() is called and the only other place I saw it called is createUnnamedGlobalFrom() I'm not familiar with this API (so please correct me if I am mistaken) but this does not look like the place to 'change address space of global variables' like the PR title suggests. This looks like code to handle cases like the integer array here - https://godbolt.org/z/nqzqx8. (Note the global this creates).

Edit: Corrected Godbolt link.

elizabethandrews · 2020-09-24T15:53:33Z

The getStringLiteralAddressSpace() function is not being used for string literal only.

Can you add tests for all the cases you're trying to support? The current test modifications only show address space change for string literals.

I took a quick look at the code to see where getStringLiteralAddressSpace() is called and the only other place I saw it called is createUnnamedGlobalFrom() I'm not familiar with this API (so please correct me if I am mistaken) but this does not look like the place to 'change address space of global variables' like the PR title suggests. This looks like code to handle cases like the integer array here - https://godbolt.org/z/nqzqx8. (Note the global this creates).

Edit: Corrected Godbolt link.

Is this the case you're trying to handle when you say global variables? I think I may have misunderstood what you meant by 'global variables'. In any case, please add a test.

fadeeval · 2020-09-25T12:30:17Z

@elizabethandrews, I am not sure that it is needed to add tests because the fixed tests already show that I wanted to do.
Arrays and static variables (as well as literal strings) are declared as global in LLVM IR, so in SPIRV it may be a problem, that this PR should fix.

premanandrao · 2020-09-25T18:00:18Z

@elizabethandrews, I am not sure that it is needed to add tests because the fixed tests already show that I wanted to do.
Arrays and static variables (as well as literal strings) are declared as global in LLVM IR, so in SPIRV it may be a problem, that this PR should fix.

Please add a test that checks this for file-scope statics, static data members, and templated versions of both. I think these will be helpful.

Signed-off-by: Aleksander Fadeev <[email protected]>

clang/lib/CodeGen/CodeGenModule.cpp

clang/test/CodeGenSYCL/static-var-address-space.cpp

Signed-off-by: Aleksander Fadeev <[email protected]>

…_wrapper * upstream/sycl: (1021 commits) [SYCL] Enable async_work_group_copy for scalar and vector bool types (intel#2582) [SYCL] Fix element type in handler::copy (intel#2590) [NFC][SYCL] Remove unnecessary if condition (intel#2585) [SYCL][NFC] Fix SYCL lit test execution on a system w/o GPU (intel#2584) [SYCL] Add error handling for non-uniform work group size case (intel#2569) [SYCL][ESIMD] Preserve undef initializer for globals in ESIMDLowerVecArg pass (intel#2555) [SYCL] Make Level-Zero events visible on the host (intel#2576) [Driver][SYCL][NFC] Add help information for -Wno-sycl-strict (intel#2570) [SYCL] Relax test to work in Win32 environment. (intel#2580) [SYCL] Emit suppressed warnings from SYCL headers (intel#2575) [SYCL][NFC] Cover more classes with ABI tests (intel#2577) [SYCL][ESIMD] Update ESIMD tests and add raw send support. (intel#2482) [SYCL] Make ESIMD on-device tests require linux,gpu,opencl. (intel#2560) [SYCL] Release commands with no dependencies after they're enqueued (intel#2492) [SYCL] Add multi-device and multi-platform support for SYCL_DEVICE_ALLOWLIST (intel#2483) [SYCL] Try to enqueue host command depencies (intel#2561) [SYCL][ESIMD][NFC] Align namespace name with the spec guidelines (intel#2573) [SYCL][NFC] Add class layout ABI tests for memory objects (intel#2559) [SYCL] Change adress space for global variables (intel#2534) [NFC][SYCL] Fix comment. (intel#2541) ...

…global [DeviceMSAN] Fix gpu crashed on device global variable

[DeviceMSAN] Fix gpu crashed on device global variable

fadeeval added 3 commits September 24, 2020 13:12

Change adress space for global variables

5c5d0ae

Signed-off-by: Aleksander Fadeev <[email protected]>

Add better solution

8b70645

Signed-off-by: Aleksander Fadeev <[email protected]>

Change tests

878f996

Signed-off-by: Aleksander Fadeev <[email protected]>

fadeeval marked this pull request as ready for review September 24, 2020 14:11

fadeeval requested review from elizabethandrews, Fznamznon and premanandrao as code owners September 24, 2020 14:11

AlexeySotkin reviewed Sep 24, 2020

View reviewed changes

bader changed the title ~~Change adress space for global variables~~ [SYCL] Change adress space for global variables Sep 28, 2020

fadeeval added 2 commits September 29, 2020 11:13

Add test and comments

d5d7290

Signed-off-by: Aleksander Fadeev <[email protected]>

Add formatting

9b50de7

Signed-off-by: Aleksander Fadeev <[email protected]>

Fznamznon reviewed Sep 29, 2020

View reviewed changes

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/static-var-address-space.cpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/static-var-address-space.cpp Outdated Show resolved Hide resolved

Fznamznon requested a review from asavonic September 29, 2020 11:49

Change comment and take single_task from sycl.hpp

52eeb92

Signed-off-by: Aleksander Fadeev <[email protected]>

Fznamznon approved these changes Sep 30, 2020

View reviewed changes

bader merged commit eac16d8 into intel:sycl Sep 30, 2020

kbenzie added a commit to kbenzie/intel-llvm that referenced this pull request Feb 17, 2025

Merge pull request intel#2534 from AllanZyne/review/yang/msan_device_…

6a3fece

…global [DeviceMSAN] Fix gpu crashed on device global variable

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2534 from AllanZyne/review/yang/msan_device_global

7220a17

[DeviceMSAN] Fix gpu crashed on device global variable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Change adress space for global variables #2534

[SYCL] Change adress space for global variables #2534

Uh oh!

fadeeval commented Sep 24, 2020 •

edited

Loading

Uh oh!

premanandrao commented Sep 24, 2020

Uh oh!

fadeeval commented Sep 24, 2020

Uh oh!

fadeeval commented Sep 24, 2020

Uh oh!

AlexeySotkin Sep 24, 2020

Uh oh!

keryell Sep 24, 2020 •

edited

Loading

Uh oh!

premanandrao Sep 24, 2020

Uh oh!

keryell Sep 25, 2020

Uh oh!

fadeeval Sep 25, 2020

Uh oh!

keryell Sep 25, 2020

Uh oh!

elizabethandrews commented Sep 24, 2020 •

edited

Loading

Uh oh!

elizabethandrews commented Sep 24, 2020

Uh oh!

fadeeval commented Sep 25, 2020

Uh oh!

premanandrao commented Sep 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[SYCL] Change adress space for global variables #2534

[SYCL] Change adress space for global variables #2534

Uh oh!

Conversation

fadeeval commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

premanandrao commented Sep 24, 2020

Uh oh!

fadeeval commented Sep 24, 2020

Uh oh!

fadeeval commented Sep 24, 2020

Uh oh!

AlexeySotkin Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premanandrao Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

fadeeval Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elizabethandrews commented Sep 24, 2020

Uh oh!

fadeeval commented Sep 25, 2020

Uh oh!

premanandrao commented Sep 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fadeeval commented Sep 24, 2020 •

edited

Loading

keryell Sep 24, 2020 •

edited

Loading

elizabethandrews commented Sep 24, 2020 •

edited

Loading