Page MenuHomePhabricator

[ThinLTO] Enable in-place symbol changes for exporting module

Authored by tejohnson on Dec 21 2015, 10:27 AM.



Move ThinLTO global value processing functions out of ModuleLinker and
into a new ThinLTOGlobalProcessor class, which performs any necessary
linkage and naming changes on the given module in place.

As a result, renameModuleForThinLTO no longer needs to create a new
Module when performing any necessary local to global promotion on a
module that we are possibly exporting from during a ThinLTO backend

During function importing the ThinLTO processing is still invoked from
the ModuleLinker (via the new class), as it needs to perform renaming and
linkage changes on the source module, e.g. in order to get the correct
renaming during local to global promotion.

Diff Detail


Event Timeline

tejohnson updated this revision to Diff 43383.Dec 21 2015, 10:27 AM
tejohnson retitled this revision from to [ThinLTO] Enable in-place symbol changes for exporting module.
tejohnson updated this object.
tejohnson added reviewers: rafael, mehdi_amini.
tejohnson added subscribers: llvm-commits, davidxl.

Once this goes in, I can change the signature of renameModuleForThinLTO so that it takes a Module reference and returns a bool instead of a unique_ptr, and then move the call out of clang (and gold-plugin in D15390) and into FunctionImportPass.

Note doing the ThinLTO promotion/renaming in a single pass over the module after function importing is done seems very difficult, for a couple reasons. One is that we need the source module information to determine how to rename during promotion. It is much easier to invoke on each source module being imported from. Also, to support ThinLTO testing via llvm-link, which does not run the pass manager.

mehdi_amini accepted this revision.Jan 4 2016, 5:58 PM
mehdi_amini edited edge metadata.

Thanks for this patch, I was complaining about the ModuleLinker doing to much stuff in a review a few weeks ago, and splitting it / extracting pieces is definitively something valuable.

130 ↗(On Diff #43383)


191 ↗(On Diff #43383)

Why was this declaration moved? Keeping it after processGlobalForThinLTO allows for a smaller diff.

This revision is now accepted and ready to land.Jan 4 2016, 5:58 PM
tejohnson added inline comments.Jan 5 2016, 9:45 AM
130 ↗(On Diff #43383)

I did (just tried again) and it doesn't touch this. What looks wrong?

191 ↗(On Diff #43383)

I think I removed this then put it back and inadvertently moved it. Will put it back.

tejohnson updated this revision to Diff 44019.Jan 5 2016, 9:51 AM
tejohnson edited edge metadata.
  • Move getLinkage declaration back to original location.
mehdi_amini added inline comments.Jan 5 2016, 10:18 AM
130 ↗(On Diff #44019)

isn't it over 80 cols?

tejohnson added inline comments.Jan 5 2016, 10:25 AM
130 ↗(On Diff #44019)

Uh, yes it is. Ok, got git clang-format to fix this. Looks like I need to specify the file name along with the commit ids to get it to format unmodified files.

tejohnson updated this revision to Diff 44020.Jan 5 2016, 10:26 AM
  • Clang format.

If you just committed, git clang-format HEAD~ should do it for every file modified by the commit. In general it take a range, so it you provide the commit hash and the commit hash matches HEAD, you're giving it an empty range.

Anything blocking before committing this?

This revision was automatically updated to reflect the committed changes.