[LTO] Enable module summary emission by default for regular LTO

[tobiasvk]

Summary

With D33921, we gained the ability to have module summaries in regular
LTO modules without triggering ThinLTO compilation. Module summaries in
regular LTO allow garbage collection (dead stripping) before LTO
compilation and thus open up additional optimization opportunities.

This patch enables summary emission in regular LTO for all targets
except ld64-based ones (which use the legacy LTO API).

[pcc]

Have you considered writing the regular LTO summaries unconditionally if -flto was specified? That was how I imagined that the interface would look.

Also, how were you planning to expose the reference graph to the linker? I gather from your message that you are teaching your linker to read the module summary index directly from bitcode files. I wonder whether it would be worth trying to avoid needing to read summaries multiple times. The approach that I had in mind was to somehow teach the linker to add regular object files to the combined summary index by creating a "global value summary" for each section, with a reference for each relocation. (This would be similar to how we add regular LTO inputs to the combined summary in D33922.) Then LTO would run as usual. Any regular object sections would then naturally participate in the summary-based dead stripping that LTO already does.

[tobiasvk]

In D34156#779270, @pcc wrote:

Have you considered writing the regular LTO summaries unconditionally if -flto was specified? That was how I imagined that the interface would look.

Absolutely, if people are OK with that. I would have enabled it by default in our tree anyway. Let me know if you prefer this (and other people if you have objections).

Also, how were you planning to expose the reference graph to the linker? I gather from your message that you are teaching your linker to read the module summary index directly from bitcode files.

Yep, pretty much. I have a layer between the linker and LLVM that implements a getRefsBySymbol API which lets the linker iterate the reference graph. We've had this out of tree for some time, it's just that your recent patch made it straightforward to implement upstream.

I wonder whether it would be worth trying to avoid needing to read summaries multiple times. The approach that I had in mind was to somehow teach the linker to add regular object files to the combined summary index by creating a "global value summary" for each section, with a reference for each relocation. (This would be similar to how we add regular LTO inputs to the combined summary in D33922.) Then LTO would run as usual. Any regular object sections would then naturally participate in the summary-based dead stripping that LTO already does.

It could be done but I'm skeptical about this from a cost/benefit perspective. We'd only save one additional read of the summaries, which is pretty cheap anyway. The GC logic in the linker is non-trivial and doesn't map particularly well to the combined summary (e.g. we'd have to deal with the liveness of groups of symbols rather than individual ones when symbols share an input section).

Thanks,
Tobias

[pcc]

In D34156#779661, @tobiasvk wrote:

In D34156#779270, @pcc wrote:

Have you considered writing the regular LTO summaries unconditionally if -flto was specified? That was how I imagined that the interface would look.

Absolutely, if people are OK with that. I would have enabled it by default in our tree anyway. Let me know if you prefer this (and other people if you have objections).

I'd be fine with it. We may want to make it conditional on the target (e.g. the summary wouldn't be much use on Darwin targets right now), but let's see what others think.

I wonder whether it would be worth trying to avoid needing to read summaries multiple times. The approach that I had in mind was to somehow teach the linker to add regular object files to the combined summary index by creating a "global value summary" for each section, with a reference for each relocation. (This would be similar to how we add regular LTO inputs to the combined summary in D33922.) Then LTO would run as usual. Any regular object sections would then naturally participate in the summary-based dead stripping that LTO already does.

It could be done but I'm skeptical about this from a cost/benefit perspective. We'd only save one additional read of the summaries, which is pretty cheap anyway. The GC logic in the linker is non-trivial and doesn't map particularly well to the combined summary (e.g. we'd have to deal with the liveness of groups of symbols rather than individual ones when symbols share an input section).

That's a fair point, and now that I think about it it does seem better for GC to be entirely driven by the linker, as you have done. Longer term we may consider replacing summary-based dead stripping entirely with linker-driven dead stripping.

[tobiasvk]

• Change patch to always emit a module summary for regular LTO

I don't see any real downside of having a summary given the marginal time and
space overhead it takes to construct and save it.

[mehdi_amini]

In D34156#780570, @tobiasvk wrote:

• Change patch to always emit a module summary for regular LTO

I don't see any real downside of having a summary given the marginal time and
space overhead it takes to construct and save it.

I think some code/logic needs to be changed in LLVM first:

bool LTOModule::isThinLTO() {
  // Right now the detection is only based on the summary presence. We may want
  // to add a dedicated flag at some point.

[tobiasvk]

In D34156#780714, @mehdi_amini wrote:

In D34156#780570, @tobiasvk wrote:

• Change patch to always emit a module summary for regular LTO

I don't see any real downside of having a summary given the marginal time and
space overhead it takes to construct and save it.

I think some code/logic needs to be changed in LLVM first:

bool LTOModule::isThinLTO() {
  // Right now the detection is only based on the summary presence. We may want
  // to add a dedicated flag at some point.

No, that check still works. hasGlobalValueSummary returns false for regular LTO summaries (since they a have different block ID in the bitcode). I believe @pcc is refactoring these APIs in a separate patch, since the name hasGlobalValueSummary doesn't quite reflect its purpose anymore.

[pcc]

In D34156#780714, @mehdi_amini wrote:

In D34156#780570, @tobiasvk wrote:

• Change patch to always emit a module summary for regular LTO

I don't see any real downside of having a summary given the marginal time and
space overhead it takes to construct and save it.

I think some code/logic needs to be changed in LLVM first:

bool LTOModule::isThinLTO() {
  // Right now the detection is only based on the summary presence. We may want
  // to add a dedicated flag at some point.

Thanks for pointing that out. Right now that function will ignore full LTO summaries because they are represented using a different record number, but once D33922 lands the hasGlobalValueSummary function will return true if the module contains a full LTO summary. I will update that function as part of D33922 to check for presence of a ThinLTO summary.

[mehdi_amini]

OK! Thanks both :)

[pcc]

Please confirm that we can still self host with full LTO now that D33922 has landed.

LGTM otherwise. Thanks!

[tobiasvk]

In D34156#781415, @pcc wrote:

Please confirm that we can still self host with full LTO now that D33922 has landed.

Good point. And the answer seems to be no :/

ld.lld: .../llvm/lib/Linker/IRMover.cpp:242: llvm::Type *(anonymous namespace)::TypeMapTy::get(llvm::Type *, SmallPtrSet<llvm::StructType *, 8> &): Assertion `!(Pair.first != Ty && Pair.second == Ty) && "mapping to a source type"' failed.

This is with cmake -DCLANG_ENABLE_BOOTSTRAP=1-DBOOTSTRAP_LLVM_ENABLE_LLD=1-DBOOTSTRAP_LLVM_ENABLE_LTO=1 (...) && ninja stage2.

[vlad.tsyrklevich]

Hi Tobias, I tracked down the failure self-hosting LLVM with LTO with this revision to https://bugs.llvm.org/show_bug.cgi?id=37684#c2 and have a fix under review in D47898. This revision needs to be updated to include the following trivial EmitSummaryIndex->PrepareForThinLTO renames to build:

--- a/lib/CodeGen/BackendUtil.cpp
+++ b/lib/CodeGen/BackendUtil.cpp
@@ -944,7 +944,7 @@ void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
   ModulePassManager MPM(CodeGenOpts.DebugPassManager);
 
   if (!CodeGenOpts.DisableLLVMPasses) {
-    bool IsThinLTO = CodeGenOpts.EmitSummaryIndex;
+    bool IsThinLTO = CodeGenOpts.PrepareForThinLTO;
     bool IsLTO = CodeGenOpts.PrepareForLTO;
 
     if (CodeGenOpts.OptimizationLevel == 0) {
diff --git a/lib/CodeGen/CGDebugInfo.cpp b/lib/CodeGen/CGDebugInfo.cpp
index db6a82b415..91f80e5739 100644
--- a/lib/CodeGen/CGDebugInfo.cpp
+++ b/lib/CodeGen/CGDebugInfo.cpp
@@ -578,7 +578,7 @@ void CGDebugInfo::CreateCompileUnit() {
                           CSInfo,
                           getSource(SM, SM.getMainFileID())),
       CGOpts.EmitVersionIdentMetadata ? Producer : "",
-      LO.Optimize || CGOpts.PrepareForLTO || CGOpts.EmitSummaryIndex,
+      LO.Optimize || CGOpts.PrepareForLTO || CGOpts.PrepareForThinLTO,
       CGOpts.DwarfDebugFlags, RuntimeVers,
       CGOpts.EnableSplitDwarf ? "" : CGOpts.SplitDwarfFile, EmissionKind,
       0 /* DWOid */, CGOpts.SplitDwarfInlining, CGOpts.DebugInfoForProfiling,

Are you still interested in landing this?

[tobiasvk]

In D34156#1125489, @vlad.tsyrklevich wrote:

Hi Tobias, I tracked down the failure self-hosting LLVM with LTO with this revision to https://bugs.llvm.org/show_bug.cgi?id=37684#c2 and have a fix under review in D47898.

Fantastic! That sounds exactly like the problem I was seeing back then. Thanks for tracking this down!

Are you still interested in landing this?

Sure, I'll push an update to the patch.

Tobias

[tobiasvk]

• Rebase for current tree
• Fix -flto -save-temps which the previous patch broke

[tobiasvk]

@pcc, @tejohnson: Are you OK with this going in now that D47898 has merged?

[tejohnson]

LGTM with following suggestions.

clang/lib/CodeGen/BackendUtil.cpp

738

Perhaps sink all this code into the use site of EmitLTOSummary below, especially because you don't need to check !CodeGenOpts.PrepareForThinLTO.

909

Ditto here.

clang/include/clang/Frontend/CodeGenOptions.def

 VALUE_CODEGENOPT(XRayInstructionThreshold , 32, 200)
 
 CODEGENOPT(InstrumentForProfiling , 1, 0) ///< Set when -pg is enabled.
 CODEGENOPT(CallFEntry , 1, 0) ///< Set when -mfentry is enabled.
 CODEGENOPT(LessPreciseFPMAD  , 1, 0) ///< Enable less precise MAD instructions to
                                      ///< be generated.
 CODEGENOPT(PrepareForLTO     , 1, 0) ///< Set when -flto is enabled on the
                                      ///< compile step.
-CODEGENOPT(EmitSummaryIndex, 1, 0)   ///< Set when -flto=thin is enabled on the
+CODEGENOPT(PrepareForThinLTO , 1, 0) ///< Set when -flto=thin is enabled on the
                                      ///< compile step.
 CODEGENOPT(LTOUnit, 1, 0) ///< Emit IR to support LTO unit features (CFI, whole
                           ///< program vtable opt).
 CODEGENOPT(IncrementalLinkerCompatible, 1, 0) ///< Emit an object file which can
                                               ///< be used with an incremental
                                               ///< linker.
 CODEGENOPT(MergeAllConstants , 1, 1) ///< Merge identical constants.
 CODEGENOPT(MergeFunctions    , 1, 0) ///< Set when -fmerge-functions is enabled.

clang/lib/CodeGen/BackendUtil.cpp

if (CodeGenOpts.OptimizationLevel <= 1) {
     PMBuilder.Inliner = createAlwaysInlinerLegacyPass(InsertLifetimeIntrinsics);
   } else {
     // We do not want to inline hot callsites for SamplePGO module-summary build
     // because profile annotation will happen again in ThinLTO backend, and we
     // want the IR of the hot path to match the profile.
     PMBuilder.Inliner = createFunctionInliningPass(
         CodeGenOpts.OptimizationLevel, CodeGenOpts.OptimizeSize,
         (!CodeGenOpts.SampleProfileFile.empty() &&
-         CodeGenOpts.EmitSummaryIndex));
+         CodeGenOpts.PrepareForThinLTO));
   }
 
   PMBuilder.OptLevel = CodeGenOpts.OptimizationLevel;
   PMBuilder.SizeLevel = CodeGenOpts.OptimizeSize;
   PMBuilder.BBVectorize = CodeGenOpts.VectorizeBB;
   PMBuilder.SLPVectorize = CodeGenOpts.VectorizeSLP;
   PMBuilder.LoopVectorize = CodeGenOpts.VectorizeLoop;
 
   PMBuilder.DisableUnrollLoops = !CodeGenOpts.UnrollLoops;
   PMBuilder.MergeFunctions = CodeGenOpts.MergeFunctions;
-  PMBuilder.PrepareForThinLTO = CodeGenOpts.EmitSummaryIndex;
+  PMBuilder.PrepareForThinLTO = CodeGenOpts.PrepareForThinLTO;
   PMBuilder.PrepareForLTO = CodeGenOpts.PrepareForLTO;
   PMBuilder.RerollLoops = CodeGenOpts.RerollLoops;
 
   MPM.add(new TargetLibraryInfoWrapperPass(*TLII));
 
   if (TM)
     TM->adjustPassManager(PMBuilder);
 
void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
   CreatePasses(PerModulePasses, PerFunctionPasses);
 
   legacy::PassManager CodeGenPasses;
   CodeGenPasses.add(
       createTargetTransformInfoWrapperPass(getTargetIRAnalysis()));
 
   std::unique_ptr<raw_fd_ostream> ThinLinkOS;
 
+  // Emit a module summary by default for Regular LTO except for ld64 targets
+  bool EmitLTOSummary =
+      (CodeGenOpts.PrepareForLTO && !CodeGenOpts.PrepareForThinLTO &&
+       llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
+           llvm::Triple::Apple);
+  if (EmitLTOSummary)
+    TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
+
   switch (Action) {
   case Backend_EmitNothing:
     break;
 
   case Backend_EmitBC:
-    if (CodeGenOpts.EmitSummaryIndex) {
+    if (CodeGenOpts.PrepareForThinLTO) {
       if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
         std::error_code EC;
         ThinLinkOS.reset(new llvm::raw_fd_ostream(
             CodeGenOpts.ThinLinkBitcodeFile, EC,
             llvm::sys::fs::F_None));
         if (EC) {
           Diags.Report(diag::err_fe_unable_to_open_output) << CodeGenOpts.ThinLinkBitcodeFile
                                                            << EC.message();
           return;
         }
       }
       PerModulePasses.add(
           createWriteThinLTOBitcodePass(*OS, ThinLinkOS.get()));
     }
     else
       PerModulePasses.add(
-          createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
+          createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
+                                  EmitLTOSummary));
     break;
 
   case Backend_EmitLL:
     PerModulePasses.add(
         createPrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   default:
if (CodeGenOpts.OptimizationLevel == 0) {
       // optimization levels into one of the distinct levels used to configure
       // the pipeline.
       PassBuilder::OptimizationLevel Level = mapToLevel(CodeGenOpts);
 
       MPM = PB.buildPerModuleDefaultPipeline(Level);
     }
   }
 
+  // Emit a module summary by default for Regular LTO except for ld64 targets
+  bool EmitLTOSummary =
+      (CodeGenOpts.PrepareForLTO && !CodeGenOpts.PrepareForThinLTO &&
+       llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
+           llvm::Triple::Apple);
+  if (EmitLTOSummary)
+    TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
+
   // FIXME: We still use the legacy pass manager to do code generation. We
   // create that pass manager here and use it as needed below.
   legacy::PassManager CodeGenPasses;
   bool NeedCodeGen = false;
   Optional<raw_fd_ostream> ThinLinkOS;
 
   // Append any output we need to the pass manager.
   switch (Action) {
   case Backend_EmitNothing:
     break;
 
   case Backend_EmitBC:
-    if (CodeGenOpts.EmitSummaryIndex) {
+    if (CodeGenOpts.PrepareForThinLTO) {
       if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
         std::error_code EC;
         ThinLinkOS.emplace(CodeGenOpts.ThinLinkBitcodeFile, EC,
                            llvm::sys::fs::F_None);
         if (EC) {
           Diags.Report(diag::err_fe_unable_to_open_output)
               << CodeGenOpts.ThinLinkBitcodeFile << EC.message();
           return;
         }
       }
       MPM.addPass(
           ThinLTOBitcodeWriterPass(*OS, ThinLinkOS ? &*ThinLinkOS : nullptr));
     } else {
       MPM.addPass(BitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-                                    CodeGenOpts.EmitSummaryIndex,
-                                    CodeGenOpts.EmitSummaryIndex));
+                                    EmitLTOSummary));
     }
     break;
 
   case Backend_EmitLL:
     MPM.addPass(PrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   case Backend_EmitAssembly:

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
                                          OPT_fno_unique_section_names, true);
 
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
 
   Opts.PrepareForLTO = Args.hasArg(OPT_flto, OPT_flto_EQ);
-  Opts.EmitSummaryIndex = false;
+  Opts.PrepareForThinLTO = false;
   if (Arg *A = Args.getLastArg(OPT_flto_EQ)) {
     StringRef S = A->getValue();
     if (S == "thin")
-      Opts.EmitSummaryIndex = true;
+      Opts.PrepareForThinLTO = true;
     else if (S != "full")
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
   }
   Opts.LTOUnit = Args.hasFlag(OPT_flto_unit, OPT_fno_lto_unit, false);
   if (Arg *A = Args.getLastArg(OPT_fthinlto_index_EQ)) {
     if (IK.getLanguage() != InputKind::LLVM_IR)
       Diags.Report(diag::err_drv_argument_only_allowed_with)
           << A->getAsString(Args) << "-x ir";

clang/test/CodeGen/emit-summary-index.c

+// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-bcanalyzer -dump | FileCheck %s
+// ; Check that the -flto=thin option emits a ThinLTO summary
+// CHECK: <GLOBALVAL_SUMMARY_BLOCK
+//
+// RUN: %clang_cc1 -flto -triple x86_64-apple-darwin -emit-llvm-bc < %s | llvm-bcanalyzer -dump | FileCheck --check-prefix=LTO %s
+// ; Check that we do not emit a summary for regular LTO on Apple platforms
+// LTO-NOT: GLOBALVAL_SUMMARY_BLOCK
+//
+// RUN: %clang_cc1 -flto -triple x86_64-pc-linux-gnu -emit-llvm-bc < %s | llvm-bcanalyzer -dump | FileCheck --check-prefix=LTOINDEX %s
+// ; Check that we emit a summary for regular LTO by default elsewhere
+// LTOINDEX: <FULL_LTO_GLOBALVAL_SUMMARY_BLOCK
+int main() {}

clang/test/Misc/thinlto.c

-// RUN: %clang_cc1 -flto=thin -emit-llvm-bc < %s | llvm-bcanalyzer -dump | FileCheck %s
-// ; Check that the -flto=thin option emits a summary
-// CHECK: <GLOBALVAL_SUMMARY_BLOCK
-int main() {}