[LTO] Add option to enable LTOUnit splitting, and disable unless needed

[tejohnson]

Summary

Adds a new -f[no]split-lto-unit flag that is disabled by default to
control module splitting during ThinLTO. It is automatically enabled
for -fsanitize=cfi and -fwhole-program-vtables.

The new EnableSplitLTOUnit codegen flag is passed down to llvm
via a new module flag of the same name.

Depends on D53890.

[tejohnson]

• As discussed off-patch, will use a new, separate option to control this

splitting, here I am using -f[no]split-lto-unit.

• Switch the default of splitting lto units to off by default, unless

compiled with CFI or -fwhole-program-vtables.

• I'll update the patch title and summary once we converge on flag/behavior.

[tejohnson]

Update a couple tests due to new default of splitting off and new flag.

[tejohnson]

In D53891#1300789, @tejohnson wrote:

• Switch the default of splitting lto units to off by default, unless

compiled with CFI or -fwhole-program-vtables.

Thinking ahead to when I add the index based WPD implementation, we are going to want to emit the type tests, etc for ThinLTO by default and keep splitting off. Would you prefer:

1. -flto=thin implies -fwhole-program-vtables, and have that latter option no longer implicitly enable splitting
2. -flto=thin directly enables the CodeGenOptions.WholeProgramVTables flag (which is only used by CGClass.cpp to insert the type tests/checked loads).
3. The code in CGClass.cpp that checks CodeGenOptions.WholeProgramVTables also checks CodeGenOptions.PrepareForThinLTO

(This assumes we want index-based WPD on by default for thinlto, which based on my measurements of minimal overhead should be fine and desirable.)

[tejohnson]

Update to use new module flag

[pcc]

You can't emit the type tests by default with -flto=thin because not all programs adhere to the conditions described in LTOVisibility.rst and we can't silently break those programs; it is something that they will need to explicitly opt in to. We should be able to emit the !type annotations by default, just not the type tests.

lib/CodeGen/BackendUtil.cpp

826

Why add this argument here (and below)?

lib/Driver/ToolChains/Clang.cpp

5112

Should this default to WholeProgramVTables || Sanitize.needsLTO() and emit an error if the user passes the (for now) unsupported combinations -fno-split-lto-unit -fwhole-program-vtables or -fno-split-lto-unit -fsanitize=cfi?

[tejohnson]

lib/CodeGen/BackendUtil.cpp

826

I think this was leftover from an earlier version where I had added a new parameter. Will revert this change (here and below).

lib/Driver/ToolChains/Clang.cpp

5112

I think the code below needs to stay as is to allow -fsplit-lto-unit to also enable the splitting even when the other options aren't on (not sure when that would be used outside of testing, but I'm assuming we want a way to force that on).

But yes I think it makes sense to emit an error on those combinations (when my follow on patches go in then we would remove the error with -fno-split-lto-unit -fwhole-program-vtables).

[tejohnson]

Implement comment suggestions. Added test for new error. Also test changes for LLVM side changes in D53890.

[tejohnson]

ping

[tejohnson]

ping - @pcc do you have any more comments or can this be committed along with D53890?

[pcc]

LGTM

include/clang/Basic/CodeGenOptions.def

120 ↗

(On Diff #179305)

traditional

121 ↗

(On Diff #179305)

devirtualization

lib/Driver/ToolChains/Clang.cpp

5112

-fsplit-lto-unit is required when linking translation units compiled with -fsanitize=cfi with translation units compiled without (the latter would need the flag).

May I suggest simplifying this code as follows:

bool RequiresSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
bool SplitLTOUnit =
 Args.hasFlag(options::OPT_fwhole_program_vtables,
	      options::OPT_fno_whole_program_vtables, RequiresSplitLTOUnit);
if (RequiresSplitLTOUnit && !SplitLTOUnit)
   error;
if (SplitLTOUnit)
  CmdArgs.push_back("-fsplit-lto-unit");

[pcc]

lib/Driver/ToolChains/Clang.cpp

5112

Sorry, this was meant to be OPT_fsplit_lto_unit and OPT_fno_split_lto_unit.

[tejohnson]

Address comments.

[hans]

Would be possible to add some documentation for this flag? From a quick search it's not clear to me what it does and when one is supposed to use it.

[nikic]

In D53891#2116541, @hans wrote:

Would be possible to add some documentation for this flag? From a quick search it's not clear to me what it does and when one is supposed to use it.

Ping on adding documentation.

[tejohnson]

In D53891#4186892, @nikic wrote:

In D53891#2116541, @hans wrote:

Would be possible to add some documentation for this flag? From a quick search it's not clear to me what it does and when one is supposed to use it.

Ping on adding documentation.

Thanks for the reminder, will do

[tejohnson]

In D53891#4186894, @tejohnson wrote:

In D53891#4186892, @nikic wrote:

In D53891#2116541, @hans wrote:

Would be possible to add some documentation for this flag? From a quick search it's not clear to me what it does and when one is supposed to use it.

Ping on adding documentation.

Thanks for the reminder, will do

Sent D145951, ptal

include/clang/Driver/Options.td

 def fvisibility_ms_compat : Flag<["-"], "fvisibility-ms-compat">, Group<f_Group>,
   HelpText<"Give global types 'default' visibility and global functions and "
            "variables 'hidden' visibility by default">;
 def fwhole_program_vtables : Flag<["-"], "fwhole-program-vtables">, Group<f_Group>,
   Flags<[CoreOption, CC1Option]>,
   HelpText<"Enables whole-program vtable optimization. Requires -flto">;
 def fno_whole_program_vtables : Flag<["-"], "fno-whole-program-vtables">, Group<f_Group>,
   Flags<[CoreOption]>;
+def fsplit_lto_unit : Flag<["-"], "fsplit-lto-unit">, Group<f_Group>,
+  Flags<[CoreOption, CC1Option]>,
+  HelpText<"Enables splitting of the LTO unit.">;
+def fno_split_lto_unit : Flag<["-"], "fno-split-lto-unit">, Group<f_Group>,
+  Flags<[CoreOption]>;
 def fforce_emit_vtables : Flag<["-"], "fforce-emit-vtables">, Group<f_Group>,
     Flags<[CC1Option]>,
     HelpText<"Emits more virtual tables to improve devirtualization">;
 def fno_force_emit_vtables : Flag<["-"], "fno-force-emit-vtables">, Group<f_Group>,
   Flags<[CoreOption]>;
 def fwrapv : Flag<["-"], "fwrapv">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Treat signed integer overflow as two's complement">;
 def fwritable_strings : Flag<["-"], "fwritable-strings">, Group<f_Group>, Flags<[CC1Option]>,

include/clang/Driver/SanitizerArgs.h

public:
   bool needsStatsRt() const { return Stats; }
   bool needsEsanRt() const {
     return Sanitizers.hasOneOf(SanitizerKind::Efficiency);
   }
   bool needsScudoRt() const { return Sanitizers.has(SanitizerKind::Scudo); }
 
   bool requiresPIE() const;
   bool needsUnwindTables() const;
+  bool needsLTO() const;
   bool linkCXXRuntimes() const { return LinkCXXRuntimes; }
   bool hasCrossDsoCfi() const { return CfiCrossDso; }
   void addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
                llvm::opt::ArgStringList &CmdArgs, types::ID InputType) const;
 };
 
 }  // namespace driver
 }  // namespace clang
 
 #endif

include/clang/Frontend/CodeGenOptions.def

 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(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(EnableSplitLTOUnit, 1, 0) ///< Enable LTO unit splitting to support
+             /// CFI and tranditional whole program
+             /// devirtulization that require whole
+             /// program IR support.
 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.
 CODEGENOPT(MSVolatile        , 1, 0) ///< Set when /volatile:ms is enabled.
 CODEGENOPT(NoCommon          , 1, 0) ///< Set when -fno-common or C++ is enabled.
 CODEGENOPT(NoDwarfDirectoryAsm , 1, 0) ///< Set when -fno-dwarf-directory-asm is

lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
   case Backend_EmitBC:
     if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
       if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
         ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
         if (!ThinLinkOS)
           return;
       }
       PerModulePasses.add(createWriteThinLTOBitcodePass(
-          *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr));
+          *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr,
+          CodeGenOpts.EnableSplitLTOUnit));
     } else {
       // Emit a module summary by default for Regular LTO except for ld64
       // targets
       bool EmitLTOSummary =
           (CodeGenOpts.PrepareForLTO &&
            !CodeGenOpts.DisableLLVMPasses &&
            llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
                llvm::Triple::Apple);
       if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
         TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 
-      PerModulePasses.add(
-          createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-                                  EmitLTOSummary));
+      PerModulePasses.add(createBitcodeWriterPass(
+          *OS, CodeGenOpts.EmitLLVMUseLists, EmitLTOSummary,
+          /*EmitModuleHash=*/false, CodeGenOpts.EnableSplitLTOUnit));
     }
     break;
 
   case Backend_EmitLL:
     PerModulePasses.add(
         createPrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
 
   case Backend_EmitBC:
     if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
       if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
         ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
         if (!ThinLinkOS)
           return;
       }
-      MPM.addPass(ThinLTOBitcodeWriterPass(*OS, ThinLinkOS ? &ThinLinkOS->os()
-                                                           : nullptr));
+      MPM.addPass(ThinLTOBitcodeWriterPass(
+          *OS, ThinLinkOS ? &ThinLinkOS->os() : nullptr,
+          CodeGenOpts.EnableSplitLTOUnit));
     } else {
       // Emit a module summary by default for Regular LTO except for ld64
       // targets
       bool EmitLTOSummary =
           (CodeGenOpts.PrepareForLTO &&
            !CodeGenOpts.DisableLLVMPasses &&
            llvm::Triple(TheModule->getTargetTriple()).getVendor() !=
                llvm::Triple::Apple);
       if (EmitLTOSummary && !TheModule->getModuleFlag("ThinLTO"))
         TheModule->addModuleFlag(Module::Error, "ThinLTO", uint32_t(0));
 
       MPM.addPass(BitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-                                    EmitLTOSummary));
+                                    EmitLTOSummary, /*EmitModuleHash=*/false,
+                                    CodeGenOpts.EnableSplitLTOUnit));
     }
     break;
 
   case Backend_EmitLL:
     MPM.addPass(PrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   case Backend_EmitAssembly:

lib/Driver/SanitizerArgs.cpp

 bool SanitizerArgs::requiresPIE() const {
   return NeedPIE || (Sanitizers.Mask & RequiresPIE);
 }
 
 bool SanitizerArgs::needsUnwindTables() const {
   return Sanitizers.Mask & NeedsUnwindTables;
 }
 
+bool SanitizerArgs::needsLTO() const { return Sanitizers.Mask & NeedsLTO; }
+
 SanitizerArgs::SanitizerArgs(const ToolChain &TC,
                              const llvm::opt::ArgList &Args) {
   SanitizerMask AllRemove = 0;  // During the loop below, the accumulated set of
                                 // sanitizers disabled by the current sanitizer
                                 // argument or any argument after it.
   SanitizerMask AllAddedKinds = 0;  // Mask of all sanitizers ever enabled by
                                     // -fsanitize= flags (directly or via group
                                     // expansion), some of which may be disabled

lib/Driver/ToolChains/Clang.cpp

   if (WholeProgramVTables) {
     if (!D.isUsingLTO())
       D.Diag(diag::err_drv_argument_only_allowed_with)
           << "-fwhole-program-vtables"
           << "-flto";
     CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  bool EnableSplitLTOUnit = Args.hasFlag(
+      options::OPT_fsplit_lto_unit, options::OPT_fno_split_lto_unit, false);
+  if (EnableSplitLTOUnit || WholeProgramVTables || Sanitize.needsLTO()) {
+    CmdArgs.push_back("-fsplit-lto-unit");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_fexperimental_isel,
                                options::OPT_fno_experimental_isel)) {
     CmdArgs.push_back("-mllvm");
     if (A->getOption().matches(options::OPT_fexperimental_isel)) {
       CmdArgs.push_back("-global-isel=1");
 
       // GISel is on by default on AArch64 -O0, so don't bother adding
       // the fallback remarks for it. Other combinations will add a warning of

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   if (Arg *A = Args.getLastArg(OPT_flto_EQ)) {
     StringRef S = A->getValue();
     if (S == "thin")
       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);
+  Opts.EnableSplitLTOUnit = Args.hasArg(OPT_fsplit_lto_unit);
   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";
     Opts.ThinLTOIndexFile = Args.getLastArgValue(OPT_fthinlto_index_EQ);
   }
   if (Arg *A = Args.getLastArg(OPT_save_temps_EQ))
     Opts.SaveTempsFilePrefix =

test/CodeGenCXX/no-lto-unit.cpp

 // RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-dis -o - %t | FileCheck %s
 // RUN: %clang_cc1 -flto=thin -flto-unit -fno-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-dis -o - %t | FileCheck %s
+// RUN: llvm-bcanalyzer -dump %t | FileCheck %s --check-prefix=NOLTOUNIT
+// NOLTOUNIT: <FLAGS op0=4/>
 
 // CHECK-NOT: !type
 class A {
   virtual void f() {}
 };
 
 A *f() {
   return new A;
 }

test/CodeGenCXX/type-metadata-thinlto.cpp

-// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: %clang_cc1 -flto=thin -flto-unit -fsplit-lto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
 // RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+// RUN: llvm-modextract -b -o - -n 1 %t | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=LTOUNIT
+// LTOUNIT: <FLAGS op0=0/>
 
 // CHECK: @_ZTV1A = linkonce_odr
 class A {
   virtual void f() {}
 };
 
 A *f() {
   return new A;
 }

test/Driver/split-lto-unit.c

+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fsplit-lto-unit 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=UNIT %s
+
+// UNIT: "-fsplit-lto-unit"
+// NOUNIT-NOT: "-fsplit-lto-unit"