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

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

Author: lamb-j

clang/include/clang/Basic/CodeGenOptions.def

@@ -309,6 +309,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.

clang/include/clang/Driver/Options.td

@@ -7092,6 +7092,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">>;

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(

clang/lib/CodeGen/BackendUtil.cpp

@@ -120,11 +120,6 @@ static cl::opt<PGOOptions::ColdFuncOpt> ClPGOColdFuncAttr(
                           "Mark cold functions with optnone.")));
 
 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."));
 } // namespace llvm
 
 namespace {
@@ -1055,11 +1050,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

clang/lib/CodeGen/CodeGenAction.cpp

@@ -60,10 +60,6 @@ using namespace llvm;
 
 #define DEBUG_TYPE "codegenaction"
 
-namespace llvm {
-extern cl::opt<bool> ClRelinkBuiltinBitcodePostop;
-}
-
 namespace clang {
 class BackendConsumer;
 class ClangDiagnosticHandler final : public DiagnosticHandler {
@@ -232,35 +228,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) {
@@ -362,7 +329,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()) {
@@ -1232,7 +1199,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

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();
 }

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