[clang][CodeGen][Comgr] Omit pre-opt link when post-opt is link requested (llvm#85672)

lamb-j · lamb-j · commit aaad45aff684 · 2024-06-17T11:13:12.000-04:00
Currently, when the -relink-builtin-bitcodes-postop option is used we
link builtin bitcodes twice: once before optimization, and again after
optimization.

With this change, we omit the pre-opt linking when the option is set,
and we rename the option to the following:

  -Xclang -mlink-builtin-bitcodes-postopt
 (-Xclang -mno-link-builtin-bitcodes-postopt)

The goal of this change is to reduce compile time. We do lose the
theoretical benefits of pre-opt linking, but in practice these are small
than the overhead of linking twice. However we may be able to address
this in a future patch by adjusting the position of the builtin-bitcode
linking pass.

Compilations not setting the option are unaffected

Comgr: update the option name used

Change-Id: Ife876232f6d3f1c1d728c16d2fec893817c5fa91
diff --git a/amd/comgr/src/comgr-compiler.cpp b/amd/comgr/src/comgr-compiler.cpp
@@ -1076,8 +1076,8 @@ amd_comgr_status_t AMDGPUCompiler::addDeviceLibraries() {
     return AMD_COMGR_STATUS_ERROR;
   }
   Args.push_back(Saver.save(Twine("--rocm-path=") + FakeRocmDir).data());
-  Args.push_back("-mllvm");
-  Args.push_back("-relink-builtin-bitcode-postop");
+  Args.push_back("-Xclang");
+  Args.push_back("-mlink-builtin-bitcode-postopt");
   NoGpuLib = false;
 
   for (auto DeviceLib : getDeviceLibraries()) {
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
@@ -298,6 +298,7 @@ CODEGENOPT(UnrollLoops       , 1, 0) ///< Control whether loops are unrolled.
 CODEGENOPT(RerollLoops       , 1, 0) ///< Control whether loops are rerolled.
 CODEGENOPT(NoUseJumpTables   , 1, 0) ///< Set when -fno-jump-tables is enabled.
 VALUE_CODEGENOPT(UnwindTables, 2, 0) ///< Unwind tables (1) or asynchronous unwind tables (2)
+CODEGENOPT(LinkBitcodePostopt, 1, 0) ///< Link builtin bitcodes after optimization pipeline.
 CODEGENOPT(VectorizeLoop     , 1, 0) ///< Run loop vectorizer.
 CODEGENOPT(VectorizeSLP      , 1, 0) ///< Run SLP vectorizer.
 CODEGENOPT(ProfileSampleAccurate, 1, 0) ///< Sample profile is accurate.
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
@@ -7005,6 +7005,11 @@ def mlink_bitcode_file : Separate<["-"], "mlink-bitcode-file">,
 def mlink_builtin_bitcode : Separate<["-"], "mlink-builtin-bitcode">,
   HelpText<"Link and internalize needed symbols from the given bitcode file "
            "before performing optimizations.">;
+defm link_builtin_bitcode_postopt: BoolMOption<"link-builtin-bitcode-postopt",
+  CodeGenOpts<"LinkBitcodePostopt">, DefaultFalse,
+  PosFlag<SetTrue, [], [ClangOption], "Link builtin bitcodes after the "
+  "optimization pipeline">,
+  NegFlag<SetFalse, [], [ClangOption]>>;
 def vectorize_loops : Flag<["-"], "vectorize-loops">,
   HelpText<"Run the Loop vectorization passes">,
   MarshallingInfoFlag<CodeGenOpts<"VectorizeLoop">>;
diff --git a/clang/lib/CodeGen/BackendConsumer.h b/clang/lib/CodeGen/BackendConsumer.h
@@ -115,10 +115,6 @@ class BackendConsumer : public ASTConsumer {
   // Links each entry in LinkModules into our module.  Returns true on error.
   bool LinkInModules(llvm::Module *M, bool ShouldLinkFiles = true);
 
-  // Load a bitcode module from -mlink-builtin-bitcode option using
-  // methods from a BackendConsumer instead of CompilerInstance
-  bool ReloadModules(llvm::Module *M);
-
   /// Get the best possible source location to represent a diagnostic that
   /// may have associated debug info.
   const FullSourceLoc getBestLocationFromDebugLoc(
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
@@ -104,11 +104,6 @@ static cl::opt<bool> ClSanitizeOnOptimizerEarlyEP(
     cl::desc("Insert sanitizers on OptimizerEarlyEP."), cl::init(false));
 
 extern cl::opt<InstrProfCorrelator::ProfCorrelatorKind> ProfileCorrelate;
-
-// Re-link builtin bitcodes after optimization
-cl::opt<bool> ClRelinkBuiltinBitcodePostop(
-    "relink-builtin-bitcode-postop", cl::Optional,
-    cl::desc("Re-link builtin bitcodes after optimization."), cl::init(false));
 } // namespace llvm
 
 namespace {
@@ -1012,11 +1007,8 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
     }
   }
 
-  // Re-link against any bitcodes supplied via the -mlink-builtin-bitcode option
-  // Some optimizations may generate new function calls that would not have
-  // been linked pre-optimization (i.e. fused sincos calls generated by
-  // AMDGPULibCalls::fold_sincos.)
-  if (ClRelinkBuiltinBitcodePostop)
+  // Link against bitcodes supplied via the -mlink-builtin-bitcode option
+  if (CodeGenOpts.LinkBitcodePostopt)
     MPM.addPass(LinkInModulesPass(BC, false));
 
   // Add a verifier pass if requested. We don't have to do this if the action
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -58,10 +58,6 @@ using namespace llvm;
 
 #define DEBUG_TYPE "codegenaction"
 
-namespace llvm {
-extern cl::opt<bool> ClRelinkBuiltinBitcodePostop;
-}
-
 namespace clang {
 class BackendConsumer;
 class ClangDiagnosticHandler final : public DiagnosticHandler {
@@ -230,35 +226,6 @@ void BackendConsumer::HandleInterestingDecl(DeclGroupRef D) {
     HandleTopLevelDecl(D);
 }
 
-bool BackendConsumer::ReloadModules(llvm::Module *M) {
-  for (const CodeGenOptions::BitcodeFileToLink &F :
-       CodeGenOpts.LinkBitcodeFiles) {
-    auto BCBuf = FileMgr.getBufferForFile(F.Filename);
-    if (!BCBuf) {
-      Diags.Report(diag::err_cannot_open_file)
-          << F.Filename << BCBuf.getError().message();
-      LinkModules.clear();
-      return true;
-    }
-
-    LLVMContext &Ctx = getModule()->getContext();
-    Expected<std::unique_ptr<llvm::Module>> ModuleOrErr =
-        getOwningLazyBitcodeModule(std::move(*BCBuf), Ctx);
-
-    if (!ModuleOrErr) {
-      handleAllErrors(ModuleOrErr.takeError(), [&](ErrorInfoBase &EIB) {
-        Diags.Report(diag::err_cannot_open_file) << F.Filename << EIB.message();
-      });
-      LinkModules.clear();
-      return true;
-    }
-    LinkModules.push_back({std::move(ModuleOrErr.get()), F.PropagateAttrs,
-                           F.Internalize, F.LinkFlags});
-  }
-
-  return false; // success
-}
-
 // Links each entry in LinkModules into our module.  Returns true on error.
 bool BackendConsumer::LinkInModules(llvm::Module *M, bool ShouldLinkFiles) {
   for (auto &LM : LinkModules) {
@@ -357,7 +324,7 @@ void BackendConsumer::HandleTranslationUnit(ASTContext &C) {
   }
 
   // Link each LinkModule into our module.
-  if (LinkInModules(getModule()))
+  if (!CodeGenOpts.LinkBitcodePostopt && LinkInModules(getModule()))
     return;
 
   for (auto &F : getModule()->functions()) {
@@ -1282,7 +1249,7 @@ void CodeGenAction::ExecuteAction() {
                          std::move(LinkModules), *VMContext, nullptr);
 
   // Link in each pending link module.
-  if (Result.LinkInModules(&*TheModule))
+  if (!CodeGenOpts.LinkBitcodePostopt && Result.LinkInModules(&*TheModule))
     return;
 
   // PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
diff --git a/clang/lib/CodeGen/LinkInModulesPass.cpp b/clang/lib/CodeGen/LinkInModulesPass.cpp
@@ -28,12 +28,8 @@ PreservedAnalyses LinkInModulesPass::run(Module &M, ModuleAnalysisManager &AM) {
   if (!BC)
     return PreservedAnalyses::all();
 
-  // Re-load bitcode modules from files
-  if (BC->ReloadModules(&M))
-    report_fatal_error("Bitcode module re-loading failed, aborted!");
-
   if (BC->LinkInModules(&M, ShouldLinkFiles))
-    report_fatal_error("Bitcode module re-linking failed, aborted!");
+    report_fatal_error("Bitcode module postopt linking failed, aborted!");
 
-  return PreservedAnalyses::all();
+  return PreservedAnalyses::none();
 }
diff --git a/clang/test/CodeGen/linking-bitcode-postopt.cpp b/clang/test/CodeGen/linking-bitcode-postopt.cpp
@@ -0,0 +1,31 @@
+// REQUIRES: amdgpu-registered-target
+
+// Test that -mlink-bitcode-postopt correctly enables LinkInModulesPass
+
+// RUN: %clang_cc1 -triple amdgcn-- -emit-llvm-bc -o /dev/null \
+// RUN:   -mllvm -print-pipeline-passes \
+// RUN: %s 2>&1 | FileCheck --check-prefixes=DEFAULT %s
+
+// DEFAULT-NOT: LinkInModulesPass
+
+// RUN: %clang_cc1 -triple amdgcn-- -emit-llvm-bc -o /dev/null \
+// RUN:   -mllvm -print-pipeline-passes \
+// RUN:   -mlink-builtin-bitcode-postopt \
+// RUN: %s 2>&1 | FileCheck --check-prefixes=OPTION-POSITIVE %s
+
+// OPTION-POSITIVE: LinkInModulesPass
+
+// RUN: %clang_cc1 -triple amdgcn-- -emit-llvm-bc -o /dev/null \
+// RUN:   -mllvm -print-pipeline-passes \
+// RUN:   -mno-link-builtin-bitcode-postopt \
+// RUN: %s 2>&1 | FileCheck --check-prefixes=OPTION-NEGATIVE %s
+
+// OPTION-NEGATIVE-NOT: LinkInModulesPass
+
+// RUN: %clang_cc1 -triple amdgcn-- -emit-llvm-bc -o /dev/null \
+// RUN:   -mllvm -print-pipeline-passes \
+// RUN:   -mlink-builtin-bitcode-postopt \
+// RUN:   -mno-link-builtin-bitcode-postopt \
+// RUN: %s 2>&1 | FileCheck --check-prefixes=OPTION-POSITIVE-NEGATIVE %s
+
+// OPTION-POSITIVE-NEGATIVE-NOT: LinkInModulesPass
