[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

827

Why add this argument here (and below)?

lib/Driver/ToolChains/Clang.cpp

5112 ↗

(On Diff #174835)

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

827

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 ↗

(On Diff #174835)

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 ↗

(On Diff #174835)

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

(On Diff #174835)

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

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.LTOUnit));
     } 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));
+                                  EmitLTOSummary, /*EmitModuleHash=*/false,
+                                  CodeGenOpts.LTOUnit));
     }
     break;
 
   case Backend_EmitLL:
     PerModulePasses.add(
         createPrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   default:
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.LTOUnit));
     } 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.LTOUnit));
     }
     break;
 
   case Backend_EmitLL:
     MPM.addPass(PrintModulePass(*OS, "", CodeGenOpts.EmitLLVMUseLists));
     break;
 
   case Backend_EmitAssembly:

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=0/>
 
 // 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: 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=4/>
 
 // CHECK: @_ZTV1A = linkonce_odr
 class A {
   virtual void f() {}
 };
 
 A *f() {
   return new A;
 }