Skip to content

[HIP][NFC] Refactor managed var codegen #85976

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

Merged
merged 1 commit into from
Mar 22, 2024
Merged

[HIP][NFC] Refactor managed var codegen #85976

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 9 additions & 18 deletions clang/lib/CodeGen/CGCUDANV.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -1160,9 +1152,8 @@ void CGNVCUDARuntime::createOffloadingEntries() {

// Returns module constructor to be added.
llvm::Function *CGNVCUDARuntime::finalizeModule() {
transformManagedVars();
Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

@yxsamliu yxsamliu Mar 22, 2024 •
edited
Loading

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

Copy link
Collaborator Author

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

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
Expand Down