[CodeGen] Add conditional to module cloning in bitcode linking #72478
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Jacob Lambert (lamb-j) ChangesNow that we have a commandline option dictating a second link step, (-relink-builtin-bitcode-postop), we can condition the module creation when linking in bitcode modules. This aims to improve performance by avoiding unnecessary linking Full diff: https://github.com/llvm/llvm-project/pull/72478.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index a7a47d17723cb73..f01a6514f6effb0 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -103,7 +103,7 @@ static cl::opt<bool> ClSanitizeOnOptimizerEarlyEP(
cl::desc("Insert sanitizers on OptimizerEarlyEP."), cl::init(false));
// Re-link builtin bitcodes after optimization
-static cl::opt<bool> ClRelinkBuiltinBitcodePostop(
+cl::opt<bool> ClRelinkBuiltinBitcodePostop(
"relink-builtin-bitcode-postop", cl::Optional,
cl::desc("Re-link builtin bitcodes after optimization."), cl::init(false));
}
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index a31a271ed77d1ca..e3ceb3adbcc93a8 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -57,6 +57,10 @@ using namespace llvm;
#define DEBUG_TYPE "codegenaction"
+namespace llvm {
+extern cl::opt<bool> ClRelinkBuiltinBitcodePostop;
+}
+
namespace clang {
class BackendConsumer;
class ClangDiagnosticHandler final : public DiagnosticHandler {
@@ -251,32 +255,50 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
}
CurLinkModule = LM.Module.get();
-
- // TODO: If CloneModule() is updated to support cloning of unmaterialized
- // modules, we can remove this
bool Err;
- if (Error E = CurLinkModule->materializeAll())
- return false;
// Create a Clone to move to the linker, which preserves the original
// linking modules, allowing them to be linked again in the future
- // TODO: Add a ShouldCleanup option to make Cloning optional. When
- // set, we can pass the original modules to the linker for cleanup
- std::unique_ptr<llvm::Module> Clone = llvm::CloneModule(*LM.Module);
+ if (ClRelinkBuiltinBitcodePostop) {
+ // TODO: If CloneModule() is updated to support cloning of unmaterialized
+ // modules, we can remove this
+ if (Error E = CurLinkModule->materializeAll())
+ return false;
- if (LM.Internalize) {
- Err = Linker::linkModules(
+ std::unique_ptr<llvm::Module> Clone = llvm::CloneModule(*LM.Module);
+
+ if (LM.Internalize) {
+ Err = Linker::linkModules(
*M, std::move(Clone), LM.LinkFlags,
[](llvm::Module &M, const llvm::StringSet<> &GVS) {
- internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
- return !GV.hasName() || (GVS.count(GV.getName()) == 0);
- });
+ internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
+ return !GV.hasName() ||
+ (GVS.count(GV.getName()) == 0);
+ });
});
- } else
- Err = Linker::linkModules(*M, std::move(Clone), LM.LinkFlags);
+ } else
+ Err = Linker::linkModules(*M, std::move(Clone), LM.LinkFlags);
- if (Err)
- return true;
+ if (Err)
+ return true;
+ }
+ // Otherwise we can link (and clean up) the original modules
+ else {
+ if (LM.Internalize) {
+ Err = Linker::linkModules(
+ *M, std::move(LM.Module), LM.LinkFlags,
+ [](llvm::Module &M, const llvm::StringSet<> &GVS) {
+ internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
+ return !GV.hasName() ||
+ (GVS.count(GV.getName()) == 0);
+ });
+ });
+ } else
+ Err = Linker::linkModules(*M, std::move(LM.Module), LM.LinkFlags);
+
+ if (Err)
+ return true;
+ }
}
return false; // success
|
|
@llvm/pr-subscribers-clang-codegen Author: Jacob Lambert (lamb-j) ChangesNow that we have a commandline option dictating a second link step, (-relink-builtin-bitcode-postop), we can condition the module creation when linking in bitcode modules. This aims to improve performance by avoiding unnecessary linking Full diff: https://github.com/llvm/llvm-project/pull/72478.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index a7a47d17723cb73..f01a6514f6effb0 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -103,7 +103,7 @@ static cl::opt<bool> ClSanitizeOnOptimizerEarlyEP(
cl::desc("Insert sanitizers on OptimizerEarlyEP."), cl::init(false));
// Re-link builtin bitcodes after optimization
-static cl::opt<bool> ClRelinkBuiltinBitcodePostop(
+cl::opt<bool> ClRelinkBuiltinBitcodePostop(
"relink-builtin-bitcode-postop", cl::Optional,
cl::desc("Re-link builtin bitcodes after optimization."), cl::init(false));
}
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index a31a271ed77d1ca..e3ceb3adbcc93a8 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -57,6 +57,10 @@ using namespace llvm;
#define DEBUG_TYPE "codegenaction"
+namespace llvm {
+extern cl::opt<bool> ClRelinkBuiltinBitcodePostop;
+}
+
namespace clang {
class BackendConsumer;
class ClangDiagnosticHandler final : public DiagnosticHandler {
@@ -251,32 +255,50 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
}
CurLinkModule = LM.Module.get();
-
- // TODO: If CloneModule() is updated to support cloning of unmaterialized
- // modules, we can remove this
bool Err;
- if (Error E = CurLinkModule->materializeAll())
- return false;
// Create a Clone to move to the linker, which preserves the original
// linking modules, allowing them to be linked again in the future
- // TODO: Add a ShouldCleanup option to make Cloning optional. When
- // set, we can pass the original modules to the linker for cleanup
- std::unique_ptr<llvm::Module> Clone = llvm::CloneModule(*LM.Module);
+ if (ClRelinkBuiltinBitcodePostop) {
+ // TODO: If CloneModule() is updated to support cloning of unmaterialized
+ // modules, we can remove this
+ if (Error E = CurLinkModule->materializeAll())
+ return false;
- if (LM.Internalize) {
- Err = Linker::linkModules(
+ std::unique_ptr<llvm::Module> Clone = llvm::CloneModule(*LM.Module);
+
+ if (LM.Internalize) {
+ Err = Linker::linkModules(
*M, std::move(Clone), LM.LinkFlags,
[](llvm::Module &M, const llvm::StringSet<> &GVS) {
- internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
- return !GV.hasName() || (GVS.count(GV.getName()) == 0);
- });
+ internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
+ return !GV.hasName() ||
+ (GVS.count(GV.getName()) == 0);
+ });
});
- } else
- Err = Linker::linkModules(*M, std::move(Clone), LM.LinkFlags);
+ } else
+ Err = Linker::linkModules(*M, std::move(Clone), LM.LinkFlags);
- if (Err)
- return true;
+ if (Err)
+ return true;
+ }
+ // Otherwise we can link (and clean up) the original modules
+ else {
+ if (LM.Internalize) {
+ Err = Linker::linkModules(
+ *M, std::move(LM.Module), LM.LinkFlags,
+ [](llvm::Module &M, const llvm::StringSet<> &GVS) {
+ internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
+ return !GV.hasName() ||
+ (GVS.count(GV.getName()) == 0);
+ });
+ });
+ } else
+ Err = Linker::linkModules(*M, std::move(LM.Module), LM.LinkFlags);
+
+ if (Err)
+ return true;
+ }
}
return false; // success
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
lamb-j
commented
Nov 16, 2023
| // TODO: Add a ShouldCleanup option to make Cloning optional. When | ||
| // set, we can pass the original modules to the linker for cleanup | ||
| std::unique_ptr<llvm::Module> Clone = llvm::CloneModule(*LM.Module); | ||
| if (ClRelinkBuiltinBitcodePostop) { |
There was a problem hiding this comment.
Open to suggestions on a cleaner way to do this. Ideally we could do something like the following, instead of duplicating the functionality twice:
std::unique_ptr<llvm::Module> LinkMod;
if (ClRelinkBuiltinBitcodePostop)
LinkMod = Clone
else
LinkMod = Original
// Link with LinkMod
But due to llvm::Module not having a default constructor, I haven't figured out a way to do something like the above. That is, creating a single variable that can represent both the Original and the Clone, and set it depending on the provided boolean.
There was a problem hiding this comment.
you could define a lambda
auto DoLink = [&](auto& Mod) { if (LM.Internalize) { Err = Linker::linkModules( *M, std::move(Mod), LM.LinkFlags, [](llvm::Module &M, const llvm::StringSet<> &GVS) { internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) { return !GV.hasName() || (GVS.count(GV.getName()) == 0); }); }); } else Err = Linker::linkModules(*M, std::move(Mod), LM.LinkFlags); };
then call this lambda
mikerice1969
approved these changes
Nov 16, 2023
lamb-j
force-pushed
the
clone-fix
branch
2 times, most recently
from
caca1f5
to
0c1b3bf
Compare
Now that we have a commandline option dictating a second link step, (-relink-builtin-bitcode-postop), we can condition the module creation when linking in bitcode modules. This aims to improve performance by avoiding unnecessary linking
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Dec 12, 2023
…72478) Now that we have a commandline option dictating a second link step, (-relink-builtin-bitcode-postop), we can condition the module creation when linking in bitcode modules. This aims to improve performance by avoiding unnecessary linking Change-Id: Icc3c897b2452c48f9250ee203b36e2afd5d69932
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that we have a commandline option dictating a second link step, (-relink-builtin-bitcode-postop), we can condition the module creation when linking in bitcode modules. This aims to improve performance by avoiding unnecessary linking