[clang] Modify linkage and register initialization of `device_global` #15148

hdelan · 2024-08-20T15:54:02Z

A SYCL device_global is initialized externally. We should make sure the
frontend registers it as such to avoid it being removed in IPO
optimizations.

Also a device_global should be given external linkage even when it
is static. If linkage is internal then the compiler may SRA a device_global,
which results in different symbols for host and device for the same
device_global.

Ping @frasercrmck @ldrumm @rafbiels

A SYCL device_global is initialized externally. Also a device_global should be given external linkage even when it is static. If linkage is internal then the compiler may SRA a device_global, which results in different symbols for host and device for the same device_global.

Make sure that static device globals work.

clang/lib/CodeGen/CodeGenModule.cpp

sycl/test-e2e/DeviceGlobal/device_global_static.cpp

clang/lib/CodeGen/CodeGenModule.cpp

Also make sure that the device global is only considered externally initialized on the device compilation pass.

hdelan · 2024-08-21T10:01:03Z

clang/lib/CodeGen/CodeGenModule.cpp

+    const RecordDecl *RD = D->getType()->getAsRecordDecl();
+    if (RD && RD->hasAttr<SYCLDeviceGlobalAttr>()) {
+      // SYCL device globals are initialized externally
+      GV->setExternallyInitialized(true);


@smanna12 do you think we can set this universally for all device_globals? In NVPTX/AMDGCN this is correct but I'm not entirely sure if we should consider device globals for SPIR-V targets to be externally initialized.

What is the change to the generated IR? I believe we attempted to achieve this by marking the device_global in llvm.compiler.used, which seems to avoid the optimizer optimizing it when it looks like it won't be initialized. I am not sure why that doesn't reach the CUDA and HIP path though.

If this works with SPIR-V too, I am curious as to whether we could drop the llvm.compiler.used marking for device_global.

I'll update the tests with the updated IR as soon as I think I have the right behaviour. I'm curious as well about the llvm.compiler.used not being honoured. It looks like the llvm.compiler.used persists until the end of CC1 phase, but it is discarded by the time llvm-link is happening. I don't know why this is. Another way to fix this behaviour would be just to make sure llvm-link is not discarding llvm.compiler.used

It turns out that file-table-tform is getting rid of the llvm.compiler.used symbol. But this is before the llc step which also involves llvm-linking libclc and libdevice for some reason. llvm-linking libclc and libdevice results in a IPO pipeline which includes some GlobalOpt stuff, which messes with the global symbols.

So I think this could be fixed just by removing linking with libclc and libdevice at the llc step. I'll verify and if that works I'll close this PR and open a new one.

We discussed this a little bit on the team, and we have decided that it is more robust not to rely on llvm.compiler.used so I am going to stick with this approach and remove the llvm.compiler.used global var for static device_globals.

I've made a patch here to modify sycl-post-link behaviour here. I think this should solve the problem

The removal of device globals from llvm.compiler.used is actually happening deliberately at sycl-post-link, not file-table-tform. Apologies for the misdirection.

No worries, but let's make sure we're on the same page: llvm.compiler.used is expected to be dropped by tools, so that behavior is reasonable. llvm.used is expected to not be dropped because it needs to survive to the final linking phase. So my suggestion is for us to use llvm.used from Clang instead of llvm.compiler.used, and then fix the tooling that was dropping llvm.used. Does that make sense?

I see, thanks for explanation. I've changed use of llvm.compiler.used to llvm.used for static device globals in the PR here. Only the NVPTX backend removes llvm[.compiler].used at lowering time, so the other backends keep the current behaviour where sycl-post-link removes the symbols for llvm.used/llvm.compiler.used.

It was because of this patchy and inconsistent handling of llvm[.compiler].used that I thought it would be better to use an approach that relied on more universally supported concepts such as linkage. But the current approach should work so I am happy keeping as is in the aforementioned PR.

As far as I'm concerned, the ideal fix would be to have all SYCL backends (NVPTX, AMDGCN and SPIR-V) treat llvm.compiler.used in the same way, by removing the global symbol at bc -> asm lowering. This is what NVPTX does currently and I think it is sensible behaviour. In this way we can make sure the symbol lasts for all IR parts of the pipeline. For device code we don't need to worry about post lowering linking optimisations that might remove the symbol, at least we don't currently need the symbol at this stage using the sycl-post-link method, so I think llvm.compiler.used is preferable to llvm.used.

In this way we would stop removing llvm.compiler.used at sycl-post-link, which is unintuitive behaviour especially given that it happens before further LLVM passes are run (not honouring llvm.compiler.used, which is supposed to last until lowering).

@AaronBallman is that a solution you can get behind?

I think I can get behind that, yeah. My biggest concern is regarding changing the linkage; so long as we don't need to do that, I'll be happy!

steffenlarsen

Test LGTM!

steffenlarsen · 2024-08-21T12:57:48Z

clang/lib/CodeGen/CodeGenModule.cpp

+    const RecordDecl *RD = D->getType()->getAsRecordDecl();
+    if (RD && RD->hasAttr<SYCLDeviceGlobalAttr>()) {
+      // SYCL device globals are initialized externally
+      GV->setExternallyInitialized(true);


What is the change to the generated IR? I believe we attempted to achieve this by marking the device_global in llvm.compiler.used, which seems to avoid the optimizer optimizing it when it looks like it won't be initialized. I am not sure why that doesn't reach the CUDA and HIP path though.

If this works with SPIR-V too, I am curious as to whether we could drop the llvm.compiler.used marking for device_global.

llvm.compiler.used is brittle and shouldn't be used in this instance. Better to make static device_globals externally linked with an alias.

Change name before making externally initialized.

elizabethandrews · 2024-08-22T17:51:31Z

clang/lib/CodeGen/CodeGenModule.cpp

        addSYCLUniqueID(GV, D, Context);
+        if (Linkage == llvm::GlobalValue::InternalLinkage) {


Hmm....This seems ok to me but I am not sure if there are repercussions I am not thinking of . @AaronBallman can you weigh in here?

clang/lib/CodeGen/CodeGenModule.cpp

Only rename the static SYCL device globals at the end of module codegen. If renaming is not deferred the compiler may generate multiple symbols for the same device global - one before renaming and another after.

- Make all global vars externally initialized - Don't use llvm.compiler.used - Change the names of static dev global vars to be prefixed with THE_PREFIX

hdelan · 2024-08-28T16:30:40Z

Ping @intel/dpcpp-cfe-reviewers

hdelan requested review from a team as code owners August 20, 2024 15:54

hdelan requested a review from steffenlarsen August 20, 2024 15:54

Add test for static device global

f5d0483

Make sure that static device globals work.

hdelan force-pushed the dev-global-static branch from e419f0d to f5d0483 Compare August 20, 2024 15:55

hdelan had a problem deploying to WindowsCILock August 20, 2024 15:56 — with GitHub Actions Error

ldrumm requested review from ldrumm and frasercrmck August 20, 2024 15:57

Update comments for clarity

86fa136

smanna12 reviewed Aug 20, 2024

View reviewed changes

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

hdelan had a problem deploying to WindowsCILock August 20, 2024 16:01 — with GitHub Actions Error

Clang format

1c6a7e2

ldrumm requested changes Aug 20, 2024

View reviewed changes

sycl/test-e2e/DeviceGlobal/device_global_static.cpp Outdated Show resolved Hide resolved

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

hdelan had a problem deploying to WindowsCILock August 20, 2024 16:11 — with GitHub Actions Failure

hdelan temporarily deployed to WindowsCILock August 20, 2024 17:19 — with GitHub Actions Inactive

Make external linkage only apply to AMDGCN/NVPTX

ffae8c0

Also make sure that the device global is only considered externally initialized on the device compilation pass.

hdelan had a problem deploying to WindowsCILock August 21, 2024 09:58 — with GitHub Actions Failure

hdelan commented Aug 21, 2024

View reviewed changes

hdelan temporarily deployed to WindowsCILock August 21, 2024 10:32 — with GitHub Actions Inactive

steffenlarsen approved these changes Aug 21, 2024

View reviewed changes

Prefix static dev global symbol with UID

ee7f6a4

hdelan had a problem deploying to WindowsCILock August 21, 2024 15:53 — with GitHub Actions Failure

hdelan had a problem deploying to WindowsCILock August 21, 2024 17:03 — with GitHub Actions Failure

Don't use llvm.compiler.used for device_globals

885bfeb

llvm.compiler.used is brittle and shouldn't be used in this instance. Better to make static device_globals externally linked with an alias.

hdelan had a problem deploying to WindowsCILock August 22, 2024 13:55 — with GitHub Actions Failure

hdelan had a problem deploying to WindowsCILock August 22, 2024 14:30 — with GitHub Actions Failure

Change order of ops

f7a3069

Change name before making externally initialized.

hdelan had a problem deploying to WindowsCILock August 22, 2024 15:31 — with GitHub Actions Failure

hdelan had a problem deploying to WindowsCILock August 22, 2024 16:26 — with GitHub Actions Failure

elizabethandrews reviewed Aug 22, 2024

View reviewed changes

frasercrmck reviewed Aug 27, 2024

View reviewed changes

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

Defer renaming of SYCL static device globals

2d80de9

Only rename the static SYCL device globals at the end of module codegen. If renaming is not deferred the compiler may generate multiple symbols for the same device global - one before renaming and another after.

hdelan had a problem deploying to WindowsCILock August 27, 2024 10:45 — with GitHub Actions Failure

hdelan had a problem deploying to WindowsCILock August 27, 2024 11:29 — with GitHub Actions Error

Update codegen test

2076358

- Make all global vars externally initialized - Don't use llvm.compiler.used - Change the names of static dev global vars to be prefixed with THE_PREFIX

hdelan had a problem deploying to WindowsCILock August 27, 2024 11:39 — with GitHub Actions Error

Clang format

ebb92a8

hdelan had a problem deploying to WindowsCILock August 27, 2024 11:48 — with GitHub Actions Failure

hdelan temporarily deployed to WindowsCILock August 27, 2024 12:26 — with GitHub Actions Inactive

Update SYCL tests

85ebf51

hdelan temporarily deployed to WindowsCILock August 27, 2024 15:34 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock August 27, 2024 16:50 — with GitHub Actions Inactive

hdelan mentioned this pull request Aug 28, 2024

[clang] Use llvm.used for Device Globals #15212

Closed

ldrumm approved these changes Aug 28, 2024

View reviewed changes

Merge branch 'sycl' into dev-global-static

0c0aacd

hdelan temporarily deployed to WindowsCILock August 28, 2024 16:33 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock August 28, 2024 17:10 — with GitHub Actions Inactive

hdelan mentioned this pull request Aug 29, 2024

[sycl-post-link] Don't remove llvm.compiler.used for NVPTX #15224

Merged

hdelan closed this Sep 3, 2024

ldrumm mentioned this pull request Dec 23, 2024

[HIP] getGlobalVariablePointer error oneapi-src/unified-runtime#2475

Closed

sherholz-intel mentioned this pull request Feb 27, 2025

[HIP] device globals amdgcn-link error #17193

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Modify linkage and register initialization of `device_global` #15148

[clang] Modify linkage and register initialization of `device_global` #15148

hdelan commented Aug 20, 2024 •

edited

Loading

hdelan Aug 21, 2024 •

edited

Loading

steffenlarsen Aug 21, 2024

hdelan Aug 21, 2024

hdelan Aug 21, 2024 •

edited

Loading

hdelan Aug 22, 2024

hdelan Aug 29, 2024

AaronBallman Aug 29, 2024

hdelan Aug 29, 2024 •

edited

Loading

hdelan Aug 29, 2024 •

edited

Loading

AaronBallman Aug 30, 2024

steffenlarsen left a comment

steffenlarsen Aug 21, 2024

elizabethandrews Aug 22, 2024

hdelan commented Aug 28, 2024

		addSYCLUniqueID(GV, D, Context);
		if (Linkage == llvm::GlobalValue::InternalLinkage) {

[clang] Modify linkage and register initialization of device_global #15148

[clang] Modify linkage and register initialization of device_global #15148

Conversation

hdelan commented Aug 20, 2024 • edited Loading

hdelan Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdelan Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdelan Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

hdelan Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steffenlarsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdelan commented Aug 28, 2024

[clang] Modify linkage and register initialization of `device_global` #15148

[clang] Modify linkage and register initialization of `device_global` #15148

hdelan commented Aug 20, 2024 •

edited

Loading

hdelan Aug 21, 2024 •

edited

Loading

hdelan Aug 21, 2024 •

edited

Loading

hdelan Aug 29, 2024 •

edited

Loading

hdelan Aug 29, 2024 •

edited

Loading