Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8106a51
Modify linkage and initialization of SYCL device_global
hdelan Aug 20, 2024
f5d0483
Add test for static device global
hdelan Aug 20, 2024
86fa136
Update comments for clarity
hdelan Aug 20, 2024
1c6a7e2
Clang format
hdelan Aug 20, 2024
ffae8c0
Make external linkage only apply to AMDGCN/NVPTX
hdelan Aug 21, 2024
ee7f6a4
Prefix static dev global symbol with UID
hdelan Aug 21, 2024
885bfeb
Don't use llvm.compiler.used for device_globals
hdelan Aug 22, 2024
f7a3069
Change order of ops
hdelan Aug 22, 2024
2d80de9
Defer renaming of SYCL static device globals
hdelan Aug 27, 2024
2076358
Update codegen test
hdelan Aug 27, 2024
ebb92a8
Clang format
hdelan Aug 27, 2024
85ebf51
Update SYCL tests
hdelan Aug 27, 2024
0c0aacd
Merge branch 'sycl' into dev-global-static
hdelan Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions clang/lib/CodeGen/CodeGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6235,6 +6235,20 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
getCUDARuntime().handleVarRegistration(D, *GV);
}

if (LangOpts.SYCLIsDevice) {
const RecordDecl *RD = D->getType()->getAsRecordDecl();
if (RD && RD->hasAttr<SYCLDeviceGlobalAttr>()) {
// SYCL device globals are initialized externally
GV->setExternallyInitialized(true);
Copy link
Contributor Author

@hdelan hdelan Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hdelan hdelan Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@hdelan hdelan Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hdelan hdelan Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

// Since static device global symbols need to cross host device boundary,
// don't allow internal linkage as it might tamper with the device global
// symbol
if (Linkage == llvm::GlobalValue::InternalLinkage &&
(getTriple().isAMDGCN() || getTriple().isNVPTX()))
Linkage = llvm::GlobalValue::ExternalLinkage;
}
}

GV->setInitializer(Init);
if (emitter)
emitter->finalize(GV);
Expand Down
31 changes: 31 additions & 0 deletions sycl/test-e2e/DeviceGlobal/device_global_static.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
//
// The OpenCL GPU backends do not currently support device_global backend
// calls.
// UNSUPPORTED: opencl && gpu
//
// Tests static device_global access through device kernels.

#include "common.hpp"

static device_global<int[4], TestProperties> DeviceGlobalVar;

int main() {
queue Q;

Q.single_task([=]() { DeviceGlobalVar.get()[0] = 42; });
// Make sure that the write happens before subsequent read
Q.wait();

int OutVal = 0;
{
buffer<int, 1> OutBuf(&OutVal, 1);
Q.submit([&](handler &CGH) {
auto OutAcc = OutBuf.get_access<access::mode::write>(CGH);
CGH.single_task([=]() { OutAcc[0] = DeviceGlobalVar.get()[0]; });
});
}
assert(OutVal == 42 && "Read value does not match.");
return 0;
}
Loading