-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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.
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
[HIP][NFC] Refactor managed var codegen #85976
Conversation
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Yaxun (Sam) Liu (yxsamliu) ChangesRefactor managed variable handling in codegen so that the transformation is done separately from registration. This will allow the new driver to register the managed var in the linker wrapper. Full diff: https://github.com/llvm/llvm-project/pull/85976.diff 1 Files Affected:
diff --git a/clang/lib/CodeGen/CGCUDANV.cpp b/clang/lib/CodeGen/CGCUDANV.cpp
index d3f2573fd5e38a..b756318c46a900 100644
--- a/clang/lib/CodeGen/CGCUDANV.cpp
+++ b/clang/lib/CodeGen/CGCUDANV.cpp
@@ -605,20 +605,10 @@ llvm::Function *CGNVCUDARuntime::makeRegisterGlobalsFn() {
uint64_t VarSize =
CGM.getDataLayout().getTypeAllocSize(Var->getValueType());
if (Info.Flags.isManaged()) {
- auto *ManagedVar = new llvm::GlobalVariable(
- CGM.getModule(), Var->getType(),
- /*isConstant=*/false, Var->getLinkage(),
- /*Init=*/Var->isDeclaration()
- ? nullptr
- : llvm::ConstantPointerNull::get(Var->getType()),
- /*Name=*/"", /*InsertBefore=*/nullptr,
- llvm::GlobalVariable::NotThreadLocal);
- ManagedVar->setDSOLocal(Var->isDSOLocal());
- ManagedVar->setVisibility(Var->getVisibility());
- ManagedVar->setExternallyInitialized(true);
- ManagedVar->takeName(Var);
- Var->setName(Twine(ManagedVar->getName() + ".managed"));
- replaceManagedVar(Var, ManagedVar);
+ assert(Var->getName().ends_with(".managed") &&
+ "HIP managed variables not transformed");
+ auto *ManagedVar = CGM.getModule().getNamedGlobal(
+ Var->getName().drop_back(StringRef(".managed").size()));
llvm::Value *Args[] = {
&GpuBinaryHandlePtr,
ManagedVar,
@@ -1093,7 +1083,9 @@ void CGNVCUDARuntime::transformManagedVars() {
: llvm::ConstantPointerNull::get(Var->getType()),
/*Name=*/"", /*InsertBefore=*/nullptr,
llvm::GlobalVariable::NotThreadLocal,
- CGM.getContext().getTargetAddressSpace(LangAS::cuda_device));
+ CGM.getContext().getTargetAddressSpace(CGM.getLangOpts().CUDAIsDevice
+ ? LangAS::cuda_device
+ : LangAS::Default));
ManagedVar->setDSOLocal(Var->isDSOLocal());
ManagedVar->setVisibility(Var->getVisibility());
ManagedVar->setExternallyInitialized(true);
@@ -1102,7 +1094,7 @@ void CGNVCUDARuntime::transformManagedVars() {
Var->setName(Twine(ManagedVar->getName()) + ".managed");
// Keep managed variables even if they are not used in device code since
// they need to be allocated by the runtime.
- if (!Var->isDeclaration()) {
+ if (CGM.getLangOpts().CUDAIsDevice && !Var->isDeclaration()) {
assert(!ManagedVar->isDeclaration());
CGM.addCompilerUsedGlobal(Var);
CGM.addCompilerUsedGlobal(ManagedVar);
@@ -1160,9 +1152,8 @@ void CGNVCUDARuntime::createOffloadingEntries() {
// Returns module constructor to be added.
llvm::Function *CGNVCUDARuntime::finalizeModule() {
+ transformManagedVars();
if (CGM.getLangOpts().CUDAIsDevice) {
- transformManagedVars();
-
// Mark ODR-used device variables as compiler used to prevent it from being
// eliminated by optimization. This is necessary for device variables
// ODR-used by host functions. Sema correctly marks them as ODR-used no
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sans the "NFC" part in the description.
@@ -1160,9 +1152,8 @@ void CGNVCUDARuntime::createOffloadingEntries() { | |||
|
|||
// Returns module constructor to be added. | |||
llvm::Function *CGNVCUDARuntime::finalizeModule() { | |||
transformManagedVars(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look like "NFC" as we now perform the transform for the host compilation, too.
I assume we do have existing tests covering generation of the variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing we also don't have a test for __managed__
in the external test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we did the equivalent transformation previously during registration of managed var by calling replaceManagedVar directly, so it is actually NFC. We have lit test https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCUDA/managed-var.cu that covers transformation in both device and host code.
We also have execution tests in hip-tests https://github.com/ROCm/hip-tests/blob/d0088ee34f1bd507e793c6fc5a148488d577c11a/catch/TypeQualifiers/hipManagedKeyword.cc#L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not have test for managed var in llvm-test-suite
Refactor managed variable handling in codegen so that the transformation is done separately from registration. This will allow the new driver to register the managed var in the linker wrapper.
11d10a8
to
1d14bcf
Compare
Refactor managed variable handling in codegen so that the transformation is done separately from registration. This will allow the new driver to register the managed var in the linker wrapper.
Refactor managed variable handling in codegen so that the transformation is done separately from registration.
This will allow the new driver to register the managed var in the linker wrapper.