Page MenuHomePhabricator

Clang support for -fthinlto.
ClosedPublic

Authored by tejohnson on Aug 10 2015, 9:40 AM.

Details

Summary

I am going to update this patch in a moment to reflect recent changes needed for the new LLVM/gold patch (D13107), and add a couple of tests.

Add clang support for -fthinlto option, which is used to set the EmitThinLTOIndex code gen option on compiles.

Dependent on LLVM/gold patch http://reviews.llvm.org/D13107.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 31677.Aug 10 2015, 9:40 AM
tejohnson retitled this revision from to Clang support for -fthinlto..
tejohnson updated this object.
tejohnson added reviewers: rafael, dexonsmith.
tejohnson added a subscriber: cfe-commits.
tejohnson updated this revision to Diff 31937.Aug 12 2015, 6:31 AM
tejohnson updated this object.

Removed gold plugin option.

davidxl added inline comments.
include/clang/Driver/Options.td
685 ↗(On Diff #31937)

spell it like "fthin-lto" seems better than one word "fthinlto"

lib/CodeGen/BackendUtil.cpp
621 ↗(On Diff #31937)

The change to this function is missing in the patch.

tejohnson added inline comments.Aug 13 2015, 10:24 AM
include/clang/Driver/Options.td
685 ↗(On Diff #31937)

I considered that too, but wanted to distinguish from regular LTO. I could go either way.

lib/CodeGen/BackendUtil.cpp
621 ↗(On Diff #31937)

Right, that is in LLVM and needs to be in a different patch (goes into a different repository). As mentioned in the patch summary this is dependent on LLVM patch http://reviews.llvm.org/D11907.

tejohnson updated this object.Sep 23 2015, 10:23 AM
tejohnson edited reviewers, added: mehdi_amini; removed: rafael.
tejohnson updated this revision to Diff 35527.Sep 23 2015, 10:28 AM

Updated the patch for the new LLVM/gold patch (D13107).

Two changes:

  • I put back the original change to pass a new plugin option to gold. Since the function index design/name has been generalized to not be ThinLTO-specific, it makes sense to have the ThinLTO-related gold behavior for modules with those sections to be dependent on an option.
  • Added 2 tests.
tejohnson updated this revision to Diff 36267.Oct 1 2015, 10:17 AM

Address review comments.

Instead of -fthinlto use -flto=thin, and add in -flto=full as an alias to the existing -flto. Add tests to check for proper overriding of -flto variants on the command line, and convert grep tests to FileCheck.

alexr added a subscriber: alexr.Oct 1 2015, 2:03 PM

I realize that we've all been referring to this as ThinLTO, but is that necessarily descriptive for users? What about inlineonly or something similar?

ThinLTO is a perfectly good term to indicate what it tries to accomplish. InlineOnly will narrow its scope. For instance, there is no reason why thinLTO can not do fast summary based WPA in the future.

klimek added a subscriber: klimek.Oct 2 2015, 6:18 AM

Perhaps "sharded" would fit what it is?

Added echristo, rafael, pcc officially as subscribers as Duncan's cc
of them via email didn't stick.

Sorry for the duplicate - my previous response didn't go to Duncan or Mehdi for some reason. Trying again...

Perhaps "sharded" would fit what it is?

You could have a sharded mode for full FDO (like gcc's partitioned LTO). And we aren't really making any explicit sharding decisions, since the backends do importing on demand.

As David mentioned, "inlineonly" is much too restrictive for what is possible. I prefer to stick with "thin" since it refers to this new model of keeping the whole program part very thin.

Does anyone have an opinion on "full" vs "monolithic" vs something else for the traditional full/monolithic LTO?

klimek added a comment.Oct 2 2015, 8:53 AM

Sorry for the duplicate - my previous response didn't go to Duncan or Mehdi for some reason. Trying again...

Perhaps "sharded" would fit what it is?

You could have a sharded mode for full FDO (like gcc's partitioned LTO). And we aren't really making any explicit sharding decisions, since the backends do importing on demand.

As David mentioned, "inlineonly" is much too restrictive for what is possible. I prefer to stick with "thin" since it refers to this new model of keeping the whole program part very thin.

Does anyone have an opinion on "full" vs "monolithic" vs something else for the traditional full/monolithic LTO?

If "sharded" is not the right term, than "monolithic" doesn't seem like the right term, either, right?

If "thin" basically refers to how much information is given to the lto steps (for which "thin" seems to be a good name actually), then "full" seems to be a good term for the, well, full information.

tejohnson updated this revision to Diff 36453.Oct 3 2015, 6:51 PM
  • Rename code gen option to EmitFunctionSummary as per review for D13107.
  • Clang format the latest changes.

    Let me know if it looks good now.

Ping.
Thanks,
Teresa

mehdi_amini accepted this revision.Oct 14 2015, 9:25 AM
mehdi_amini edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 14 2015, 9:25 AM
This revision was automatically updated to reflect the committed changes.