Page MenuHomePhabricator

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

Authored by tejohnson on Oct 30 2018, 1:34 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson created this revision.Oct 30 2018, 1:34 PM
tejohnson updated this revision to Diff 174329.Nov 15 2018, 9:46 PM
  • 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.

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

  • 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 updated this revision to Diff 174835.Nov 20 2018, 1:52 PM

Update to use new module flag

tejohnson retitled this revision from [LTO] Pass down LTOUnit codegen flag to bitcode writer to [LTO] Add option to enable LTOUnit splitting, and disable unless needed.Nov 20 2018, 2:04 PM
tejohnson edited the summary of this revision. (Show Details)
ormris added a subscriber: ormris.Nov 20 2018, 2:08 PM
pcc added a comment.Dec 18 2018, 9:21 PM

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
831 ↗(On Diff #174835)

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 marked 2 inline comments as done.Dec 19 2018, 8:51 AM
tejohnson added inline comments.
lib/CodeGen/BackendUtil.cpp
831 ↗(On Diff #174835)

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 marked 2 inline comments as done.Dec 21 2018, 9:43 AM
tejohnson updated this revision to Diff 179305.Dec 21 2018, 9:56 AM

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

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

pcc accepted this revision.Jan 10 2019, 4:33 PM

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");
This revision is now accepted and ready to land.Jan 10 2019, 4:33 PM
pcc added inline comments.Jan 10 2019, 4:34 PM
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 marked 4 inline comments as done.

Address comments.

This revision was automatically updated to reflect the committed changes.