[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 ↗

(On Diff #169740)

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

2

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

Nit: line length

20

Nit: line length

[trentxintong]

Address @tejohnson suggestions.

[tejohnson]

LGTM with some minor comment suggestions

test/LTO/X86/internalize.ll

8

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.

19

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

test/ThinLTO/X86/internalize.ll

7

Same suggestion as above

20

Same suggestion as above

[trentxintong]

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

include/llvm/LTO/legacy/LTOCodeGenerator.h

 #include "llvm/IR/Module.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include <string>
 #include <vector>
 
+/// Enable global value internalization in LTO.
+extern llvm::cl::opt<bool> EnableLTOInternalization;
+
 namespace llvm {
 template <typename T> class ArrayRef;
   class LLVMContext;
   class DiagnosticInfo;
   class Linker;
   class Mangler;
   class MemoryBuffer;
   class TargetLibraryInfo;
private:
   std::string NativeObjectPath;
   TargetOptions Options;
   CodeGenOpt::Level CGOptLevel = CodeGenOpt::Default;
   const Target *MArch = nullptr;
   std::string TripleStr;
   unsigned OptLevel = 2;
   lto_diagnostic_handler_t DiagHandler = nullptr;
   void *DiagContext = nullptr;
-  bool ShouldInternalize = true;
+  bool ShouldInternalize = EnableLTOInternalization;
   bool ShouldEmbedUselists = false;
   bool ShouldRestoreGlobalsLinkage = false;
   TargetMachine::CodeGenFileType FileType = TargetMachine::CGFT_ObjectFile;
   std::unique_ptr<ToolOutputFile> DiagnosticOutputFile;
   bool Freestanding = false;
 };
 }
 #endif

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,

test/LTO/X86/internalize.ll

+; RUN: opt %s -o %t1.bc
+
+; RUN: llvm-lto %t1.bc -o %t1.save.opt  --exported-symbol=_foo -save-merged-module -O0
+; RUN: llvm-dis < %t1.save.opt.merged.bc | FileCheck %s --check-prefix=INTERNALIZE
+
+; Test the enable-lto-internalization option by setting it to false.
+; This makes sure internalization does not happen.
+; RUN: llvm-lto %t1.bc -enable-lto-internalization=false -o %t1.save.opt  \
+; RUN:                 --exported-symbol=_foo -save-merged-module -O0
+; RUN: llvm-dis < %t1.save.opt.merged.bc | FileCheck %s --check-prefix=INTERNALIZE-OPTION-DISABLE
+
+; RUN: llvm-lto2 run %t1.bc -o %t.o -save-temps \
+; RUN:     -r=%t1.bc,_foo,pxl \
+; RUN:     -r=%t1.bc,_bar,pl
+; RUN: llvm-dis < %t.o.0.2.internalize.bc | FileCheck  %s --check-prefix=INTERNALIZE2
+
+; Test the enable-lto-internalization option by setting it to false.
+; This makes sure internalization does not happen in runRegularLTO().
+; RUN: llvm-lto2 run %t1.bc -o %t.o -save-temps -enable-lto-internalization=false \
+; RUN:     -r=%t1.bc,_foo,pxl \
+; RUN:     -r=%t1.bc,_bar,pl
+; RUN: llvm-dis < %t.o.0.2.internalize.bc | FileCheck  %s --check-prefix=INTERNALIZE2-OPTION-DISABLE
+
+; INTERNALIZE: define void @foo
+; INTERNALIZE: define internal void @bar
+; INTERNALIZE-OPTION-DISABLE: define void @foo
+; INTERNALIZE-OPTION-DISABLE: define void @bar
+; INTERNALIZE2: define dso_local void @foo
+; INTERNALIZE2: define internal void @bar
+; INTERNALIZE2-OPTION-DISABLE: define dso_local void @foo
+; INTERNALIZE2-OPTION-DISABLE: define dso_local void @bar
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+define void @foo() {
+    call void @bar()
+    ret void
+}
+define void @bar() {
+    ret void
+}

test/ThinLTO/X86/internalize.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc
 ; RUN: llvm-lto -thinlto-action=internalize -thinlto-index %t.index.bc %t1.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=REGULAR
 ; RUN: llvm-lto -thinlto-action=internalize -thinlto-index %t.index.bc %t1.bc -o -  --exported-symbol=foo | llvm-dis -o - | FileCheck %s --check-prefix=INTERNALIZE
 
+; Test the enable-lto-internalization option by setting it to false.
+; This makes sure indices are not marked as internallinkage and therefore
+; internalization does not happen.
+; RUN: llvm-lto -thinlto-action=internalize -thinlto-index %t.index.bc %t1.bc \
+; RUN:          -enable-lto-internalization=false --exported-symbol=foo
+; RUN: llvm-dis < %t1.bc.thinlto.internalized.bc | FileCheck %s --check-prefix=INTERNALIZE-OPTION-DISABLE
+
 ; RUN: llvm-lto2 run %t1.bc -o %t.o -save-temps \
 ; RUN:     -r=%t1.bc,_foo,pxl \
 ; RUN:     -r=%t1.bc,_bar,pl \
 ; RUN:     -r=%t1.bc,_linkonce_func,pl
 ; RUN: llvm-dis < %t.o.1.2.internalize.bc | FileCheck  %s --check-prefix=INTERNALIZE2
 
+; Test the enable-lto-internalization option by setting it to false.
+; This makes sure indices are not marked as internallinkage and therefore
+; internalization does not happen.
+; RUN: llvm-lto2 run %t1.bc -o %t.o -save-temps -enable-lto-internalization=false \
+; RUN:     -r=%t1.bc,_foo,pxl \
+; RUN:     -r=%t1.bc,_bar,pl \
+; RUN:     -r=%t1.bc,_linkonce_func,pl
+; RUN: llvm-dis < %t.o.1.2.internalize.bc | FileCheck  %s --check-prefix=INTERNALIZE2-OPTION-DISABLE
 
 ; REGULAR: define void @foo
 ; REGULAR: define void @bar
 ; REGULAR: define linkonce void @linkonce_func()
 ; INTERNALIZE: define void @foo
 ; INTERNALIZE: define internal void @bar
 ; INTERNALIZE: define internal void @linkonce_func()
+; INTERNALIZE-OPTION-DISABLE: define void @foo
+; INTERNALIZE-OPTION-DISABLE: define void @bar
+; INTERNALIZE-OPTION-DISABLE: define linkonce void @linkonce_func()
 ; INTERNALIZE2: define dso_local void @foo
 ; INTERNALIZE2: define internal void @bar
 ; INTERNALIZE2: define internal void @linkonce_func()
+; INTERNALIZE2-OPTION-DISABLE: define dso_local void @foo
+; INTERNALIZE2-OPTION-DISABLE: define dso_local void @bar
+; INTERNALIZE2-OPTION-DISABLE: define weak dso_local void @linkonce_func()
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 define void @foo() {
     call void @bar()
     ret void
 }
 define void @bar() {
     call void @linkonce_func()
   ret void
 }
 define linkonce void @linkonce_func() {
     ret void
 }