[clang] [CodeGen] Add conditional to module cloning in bitcode linking (PR #72478)

Jacob Lambert via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 29 12:55:40 PST 2023


https://github.com/lamb-j updated https://github.com/llvm/llvm-project/pull/72478

>From 7d2ee902f1fb1dc0cd355b4b51fda32309f45ccb Mon Sep 17 00:00:00 2001
From: Jacob Lambert <jacob.lambert at amd.com>
Date: Wed, 15 Nov 2023 22:06:46 -0800
Subject: [PATCH 1/5] [CodeGen] Add conditional to module cloning in bitcode
 linking

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
---
 clang/lib/CodeGen/BackendUtil.cpp   |  2 +-
 clang/lib/CodeGen/CodeGenAction.cpp | 56 ++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 18 deletions(-)

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

>From 9fb0b81d2d4770f8d5c23361e7fb3dc135a49355 Mon Sep 17 00:00:00 2001
From: Jacob Lambert <jacob.lambert at amd.com>
Date: Thu, 16 Nov 2023 07:29:55 -0800
Subject: [PATCH 2/5] [NFC] Taking clang-format suggestions

---
 clang/lib/CodeGen/CodeGenAction.cpp | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index e3ceb3adbcc93a8..fbf53accd48d0b2 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -271,10 +271,9 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
         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);
@@ -288,10 +287,9 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
         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);
-                            });
+            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);

>From 96f74031576b47992bdb110620b284f26e6543bf Mon Sep 17 00:00:00 2001
From: Jacob Lambert <jacob.lambert at amd.com>
Date: Thu, 16 Nov 2023 10:59:38 -0800
Subject: [PATCH 3/5] [NFC] More clang format fixes

---
 clang/lib/CodeGen/CodeGenAction.cpp | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index fbf53accd48d0b2..0c8ea52f4babba2 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -269,12 +269,12 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
 
       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);
+            *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);
+              });
             });
-          });
       } else
         Err = Linker::linkModules(*M, std::move(Clone), LM.LinkFlags);
 
@@ -285,12 +285,12 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
     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);
+            *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);
 

>From 0c1b3bff8e31bd171aa09e52bb8f0c75c4018e0e Mon Sep 17 00:00:00 2001
From: Jacob Lambert <jacob.lambert at amd.com>
Date: Wed, 29 Nov 2023 11:32:26 -0800
Subject: [PATCH 4/5] Refactor conditional to use lambda

---
 clang/lib/CodeGen/CodeGenAction.cpp | 41 +++++++++++------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index 0c8ea52f4babba2..bb6b1a3bc228cf9 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -257,6 +257,19 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
     CurLinkModule = LM.Module.get();
     bool Err;
 
+    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);
+    };
+
     // Create a Clone to move to the linker, which preserves the original
     // linking modules, allowing them to be linked again in the future
     if (ClRelinkBuiltinBitcodePostop) {
@@ -267,35 +280,11 @@ bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
 
       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);
-              });
-            });
-      } else
-        Err = Linker::linkModules(*M, std::move(Clone), LM.LinkFlags);
-
-      if (Err)
-        return true;
+      DoLink(Clone);
     }
     // 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;
+      DoLink(LM.Module);
     }
   }
 



More information about the cfe-commits mailing list