Skip to content

[HIP] Do not include the CUID module hash with the new driver#84332

Merged
jhuber6 merged 1 commit into
llvm:mainfrom
jhuber6:HIPHash
Mar 7, 2024
Merged

[HIP] Do not include the CUID module hash with the new driver#84332
jhuber6 merged 1 commit into
llvm:mainfrom
jhuber6:HIPHash

Conversation

@jhuber6

@jhuber6 jhuber6 commented Mar 7, 2024

Copy link
Copy Markdown
Contributor

Summary:
The new driver does not need this hash and it can lead to redefined
symbol errors when the CUID hash isn't set.

Summary:
The new driver does not need this hash and it can lead to redefined
symbol errors when the CUID hash isn't set.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Mar 7, 2024
@llvmbot

llvmbot commented Mar 7, 2024

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Joseph Huber (jhuber6)

Changes

Summary:
The new driver does not need this hash and it can lead to redefined
symbol errors when the CUID hash isn't set.


Full diff: https://github.com/llvm/llvm-project/pull/84332.diff

1 Files Affected:

  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+1-1)
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index d02875c6a86d77..967319bdfc4571 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -916,7 +916,7 @@ void CodeGenModule::Release() {
         llvm::ConstantArray::get(ATy, UsedArray), "__clang_gpu_used_external");
     addCompilerUsedGlobal(GV);
   }
-  if (LangOpts.HIP) {
+  if (LangOpts.HIP && !getLangOpts().OffloadingNewDriver) {
     // Emit a unique ID so that host and device binaries from the same
     // compilation unit can be associated.
     auto *GV = new llvm::GlobalVariable(

@yxsamliu

yxsamliu commented Mar 7, 2024

Copy link
Copy Markdown
Contributor

CUID is needed for device static variable to be accessible on host side. Since the driver does not know whether device static variables are accessed on host side, it should always enable CUID for HIP.

@jhuber6

jhuber6 commented Mar 7, 2024

Copy link
Copy Markdown
Contributor Author

CUID is needed for device static variable to be accessible on host side. Since the driver does not know whether device static variables are accessed on host side, it should always enable CUID for HIP.

Yeah that's what I was wondering about when I noticed that this CUID thing wasn't being set when @Meinersbur was testing on Windows. However, I don't think we need this module tag regardless.

@jhuber6

jhuber6 commented Mar 7, 2024

Copy link
Copy Markdown
Contributor Author

CUID is needed for device static variable to be accessible on host side. Since the driver does not know whether device static variables are accessed on host side, it should always enable CUID for HIP.

Oh! I think I remember what I did. I made the CUID hash generator thing make a fallback that just uses the file's inode because it was likely good enough for the use-case. So the new driver doesn't pass -cuid but it does do some scrambling on the globals.

@yxsamliu

yxsamliu commented Mar 7, 2024

Copy link
Copy Markdown
Contributor

CUID is needed for device static variable to be accessible on host side. Since the driver does not know whether device static variables are accessed on host side, it should always enable CUID for HIP.

Oh! I think I remember what I did. I made the CUID hash generator thing make a fallback that just uses the file's inode because it was likely good enough for the use-case. So the new driver doesn't pass -cuid but it does do some scrambling on the globals.

As long as each generated .o has a unique CUID that is fine. Using inode as CUID has issue since users may compile the same file twice with different options to generate two different .o file (the case of rccl), then the CUID is not unique.

@jhuber6
jhuber6 merged commit 630289f into llvm:main Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants