[ThinLTO] Add an option to disable (thin)lto internalization.

[trentxintong]

Summary

LTO and ThinLTO optimizes the IR differently.

One source of differences is the amount of internalizations that
can happen.

Add an option to enable/disable internalization so that other
differences can be studied in isolation. e.g. inlining.

There are other things lto and thinlto do differently, I will add
flags to enable/disable them as needed.

[trentxintong]

Internalization leads to the possibility of changing the function ABI, better global modref AA, better chance of discarding an unused symbol, etc.

ThinLTO usually does not do as good of a job internalizing than LTO.

With this option, we can study the effect of other optimizations, primarily importing+inlining, in (mostly) isolation. i.e. With it disabled, we can study the performance gap between LTO and ThinLTO because of differences in importing and inlining decisions in isolation.

Another benefit we could get is if we disable internalization and LTO, ThinLTO binary performs very similarly (or less differently than before). We can then focus on looking at what have been internalized and what benefits those bring.

[tejohnson]

The ThinLTO code is shared between the new and old LTO APIs (via thinLTOInternalizeAndPromoteInIndex), but the regular LTO internalization is not. Suggest sharing this option with lib/LTO/LTOCodeGenerator.cpp and guarding the regular LTO internalization there too to cover the old LTO API case.

[trentxintong]

Address @tejohnson suggestions. Thanks!

[tejohnson]

lib/LTO/LTOCodeGenerator.cpp

425

How about initialize ShouldInternalize in the constructor based on that option value?

[trentxintong]

Address @tejohnson suggesions. Thanks!

[tejohnson]

Code looks fine now but just realized there is no test. You can probably create one test with a function that can normally be internalized, and then try it for the 4 combinations of LTO/ThinLTO and new/old LTO with and without your option. See test/ThinLTO/X86/internalize.ll for a ThinLTO-specific internalization test that tests both the old (via llvm-lto) and the new (via llvm-lto2) LTO APIs.

[trentxintong]

Sure. I will add those tests. Thanks for spending time reviewing this @tejohnson!

[trentxintong]

Address @tejohson's suggestion to add test cases.

I've added 3/4 suggested scenarios and could not figure out how to add LTO with llvm-lto
More specifically, it seems to me that llvm-lto does not go through the runRegularLTO path
(where GVs are internalized) when processing LTO modules.

[tejohnson]

In D53294#1276434, @trentxintong wrote:

Address @tejohson's suggestion to add test cases.

I've added 3/4 suggested scenarios and could not figure out how to add LTO with llvm-lto
More specifically, it seems to me that llvm-lto does not go through the runRegularLTO path
(where GVs are internalized) when processing LTO modules.

That's because llvm-lto invokes the old LTO API (i.e the interfaces in LTO/legacy/LTOCodeGenerator.h), whereas llvm-lto2 invokes the new LTO API (which includes runRegularLTO).

To invoke regular LTO instead of ThinLTO you should not use -module-summary with opt (see my comment in your new test attempting to do regular LTO). You should be able to test regular LTO with the old LTO API. I.e. your change to LTOCodeGenerator.h should cause LTOCodeGenerator::applyScopeRestrictions to exit before invoking internalizeModule.

Note that ThinLTO for both old and new LTO APIs invokes the same thinLTOInternalizeAndPromoteInIndex routine, it's only regular LTO that has very different support for doing the internalization.

test/LTO/X86/internalize.ll

1 ↗

(On Diff #171173)

Don't use -module-summary for testing regular LTO. With a module summary, the invocations of llvm-lto2 below are both doing ThinLTO.

test/ThinLTO/X86/internalize.ll

7 ↗

(On Diff #171173)

Nit: line length

17 ↗

(On Diff #171173)

Nit: line length

[trentxintong]

Address @tejohnson suggestions.

[tejohnson]

LGTM with some minor comment suggestions

test/LTO/X86/internalize.ll

7 ↗

(On Diff #172525)

Either change to "enable-lto-internalization=false makes sure..." or maybe just change to "This make sure..." for simplicity, since you already mention the option name on the prior line.

18 ↗

(On Diff #172525)

Same suggestion: "This makes sure..." (will also fix line length which seems too long).

test/ThinLTO/X86/internalize.ll

7 ↗

(On Diff #172525)

Same suggestion as above

20 ↗

(On Diff #172525)

Same suggestion as above

[trentxintong]

Update last bit of suggestions. Thanks for review @tejohnson.

lib/LTO/LTO.cpp

 using namespace object;
 
 #define DEBUG_TYPE "lto"
 
 static cl::opt<bool>
     DumpThinCGSCCs("dump-thin-cg-sccs", cl::init(false), cl::Hidden,
                    cl::desc("Dump the SCCs in the ThinLTO index's callgraph"));
 
+/// Enable global value internalization in LTO.
+cl::opt<bool> EnableLTOInternalization(
+    "enable-lto-internalization", cl::init(true), cl::Hidden,
+    cl::desc("Enable global value internalization in LTO"));
+
 // Returns a unique hash for the Module considering the current list of
 // export/import and other global analysis results.
 // The hash is produced in \p Key.
 static void computeCacheKey(
     SmallString<40> &Key, const Config &Conf, const ModuleSummaryIndex &Index,
     StringRef ModuleID, const FunctionImporter::ImportMapTy &ImportList,
     const FunctionImporter::ExportSetTy &ExportList,
     const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,

 
 static void thinLTOInternalizeAndPromoteGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported) {
   for (auto &S : GVSummaryList) {
     if (isExported(S->modulePath(), GUID)) {
       if (GlobalValue::isLocalLinkage(S->linkage()))
         S->setLinkage(GlobalValue::ExternalLinkage);
-    } else if (!GlobalValue::isLocalLinkage(S->linkage()))
+    } else if (EnableLTOInternalization &&
+               !GlobalValue::isLocalLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
   }
 }
 
 // Update the linkages in the given \p Index to mark exported values
 // as external and non-exported values as internal.
 void llvm::thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
for (const auto &R : GlobalResolutions) {
       GlobalValue *GV =
           RegularLTO.CombinedModule->getNamedValue(R.second.IRName);
       // Ignore symbols defined in other partitions.
       // Also skip declarations, which are not allowed to have internal linkage.
       if (!GV || GV->hasLocalLinkage() || GV->isDeclaration())
         continue;
       GV->setUnnamedAddr(R.second.UnnamedAddr ? GlobalValue::UnnamedAddr::Global
                                               : GlobalValue::UnnamedAddr::None);
-      if (R.second.Partition == 0)
+      if (EnableLTOInternalization && R.second.Partition == 0)
         GV->setLinkage(GlobalValue::InternalLinkage);
     }
 
     if (Conf.PostInternalizeModuleHook &&
         !Conf.PostInternalizeModuleHook(0, *RegularLTO.CombinedModule))
       return Error::success();
   }
   return backend(Conf, AddStream, RegularLTO.ParallelCodeGenParallelismLevel,

lib/LTO/LTOCodeGenerator.cpp

 const char* LTOCodeGenerator::getVersionString() {
 #ifdef LLVM_VERSION_INFO
   return PACKAGE_NAME " version " PACKAGE_VERSION ", " LLVM_VERSION_INFO;
 #else
   return PACKAGE_NAME " version " PACKAGE_VERSION;
 #endif
 }
 
+/// Enable global value internalization in LTO.
+extern cl::opt<bool> EnableLTOInternalization;
+
 namespace llvm {
 cl::opt<bool> LTODiscardValueNames(
     "lto-discard-value-names",
     cl::desc("Strip names from Value during LTO (other than GlobalValue)."),
 #ifdef NDEBUG
     cl::init(true),
 #else
     cl::init(false),
auto mustPreserveGV = [&](const GlobalValue &GV) -> bool {
     MangledName.reserve(GV.getName().size() + 1);
     Mang.getNameWithPrefix(MangledName, &GV, /*CannotUsePrivateLabel=*/false);
     return MustPreserveSymbols.count(MangledName);
   };
 
   // Preserve linkonce value on linker request
   preserveDiscardableGVs(*MergedModule, mustPreserveGV);
 
-  if (!ShouldInternalize)
+  if (!EnableLTOInternalization || !ShouldInternalize)
     return;
 
   if (ShouldRestoreGlobalsLinkage) {
     // Record the linkage type of non-local symbols so they can be restored
     // prior
     // to module splitting.
     auto RecordLinkage = [&](const GlobalValue &GV) {
       if (!GV.hasAvailableExternallyLinkage() && !GV.hasLocalLinkage() &&
void LTOCodeGenerator::applyScopeRestrictions() {
 
   internalizeModule(*MergedModule, mustPreserveGV);
 
   ScopeRestrictionsDone = true;
 }
 
 /// Restore original linkage for symbols that may have been internalized
 void LTOCodeGenerator::restoreLinkageForExternals() {
-  if (!ShouldInternalize || !ShouldRestoreGlobalsLinkage)
+  if (!EnableLTOInternalization || !ShouldInternalize ||
+      !ShouldRestoreGlobalsLinkage)
     return;
 
   assert(ScopeRestrictionsDone &&
          "Cannot externalize without internalization!");
 
   if (ExternalSymbols.empty())
     return;