This is an archive of the discontinued LLVM Phabricator instance.

[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

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
836

Why add this argument here (and below)?

lib/Driver/ToolChains/Clang.cpp
5210

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
836

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
5210

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

traditional

121

devirtualization

lib/Driver/ToolChains/Clang.cpp
5210

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

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.
hans added a subscriber: hans.Jun 26 2020, 3:54 AM

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 added a subscriber: nikic.Mar 11 2023, 2:21 PM

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.

Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 2:21 PM

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

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