Skip to content

[LinkerWrapper] Correctly handle multiple image wrappers #67679

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

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Sep 28, 2023

Summary:
We use these image wrappers to do runtime specifica registration of
variables and to load the device image that was compiled. This was
intended to support multiple of these running at the same time, e.g. you
can have a CUDA instance running with OpenMP and they should both
function so long as you do not share state between the two. However,
because we did not use a unique name for this file it would cause
conflicts when included. This patch names the image based off of the
language runtime it's using so that they remain separate.

Fixes: #67583

Summary:
We use these image wrappers to do runtime specifica registration of
variables and to load the device image that was compiled. This was
intended to support multiple of these running at the same time, e.g. you
can have a CUDA instance running with OpenMP and they should both
function so long as you do not share state between the two. However,
because we did not use a unique name for this file it would cause
conflicts when included. This patch names the image based off of the
language runtime it's using so that they remain separate.

Fixes: llvm#67583
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Sep 28, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 28, 2023

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Changes

Summary:
We use these image wrappers to do runtime specifica registration of
variables and to load the device image that was compiled. This was
intended to support multiple of these running at the same time, e.g. you
can have a CUDA instance running with OpenMP and they should both
function so long as you do not share state between the two. However,
because we did not use a unique name for this file it would cause
conflicts when included. This patch names the image based off of the
language runtime it's using so that they remain separate.

Fixes: #67583


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

1 Files Affected:

  • (modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+6-4)
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index b5f6fb35f227321..632e37e3cac8fec 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -810,7 +810,7 @@ Expected<StringRef> writeOffloadFile(const OffloadFile &File) {
 
 // Compile the module to an object file using the appropriate target machine for
 // the host triple.
-Expected<StringRef> compileModule(Module &M) {
+Expected<StringRef> compileModule(Module &M, OffloadKind Kind) {
   llvm::TimeTraceScope TimeScope("Compile module");
   std::string Msg;
   const Target *T = TargetRegistry::lookupTarget(M.getTargetTriple(), Msg);
@@ -829,8 +829,10 @@ Expected<StringRef> compileModule(Module &M) {
     M.setDataLayout(TM->createDataLayout());
 
   int FD = -1;
-  auto TempFileOrErr = createOutputFile(
-      sys::path::filename(ExecutableName) + ".image.wrapper", "o");
+  auto TempFileOrErr =
+      createOutputFile(sys::path::filename(ExecutableName) + "." +
+                           getOffloadKindName(Kind) + ".image.wrapper",
+                       "o");
   if (!TempFileOrErr)
     return TempFileOrErr.takeError();
   if (std::error_code EC = sys::fs::openFileForWrite(*TempFileOrErr, FD))
@@ -902,7 +904,7 @@ wrapDeviceImages(ArrayRef<std::unique_ptr<MemoryBuffer>> Buffers,
     WriteBitcodeToFile(M, OS);
   }
 
-  auto FileOrErr = compileModule(M);
+  auto FileOrErr = compileModule(M, Kind);
   if (!FileOrErr)
     return FileOrErr.takeError();
   return *FileOrErr;

@shiltian
Copy link
Contributor

Can we have a test?

@llvmbot llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Sep 28, 2023
@jhuber6
Copy link
Contributor Author

jhuber6 commented Sep 28, 2023

Can we have a test?

Sure, the reason that it failed w/ -save-temps is because normally the output name got a temp file but -save-temps keeps it the same name.

@jhuber6 jhuber6 merged commit 7485d36 into llvm:main Sep 28, 2023
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
Summary:
We use these image wrappers to do runtime specifica registration of
variables and to load the device image that was compiled. This was
intended to support multiple of these running at the same time, e.g. you
can have a CUDA instance running with OpenMP and they should both
function so long as you do not share state between the two. However,
because we did not use a unique name for this file it would cause
conflicts when included. This patch names the image based off of the
language runtime it's using so that they remain separate.

Fixes: llvm#67583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OpenMP] Multiple definition error with --save-temps when linking CUDA device code
3 participants