This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Only promote exported locals as marked in index
ClosedPublic

Authored by tejohnson on Nov 9 2016, 1:26 PM.

Details

Summary

We have always speculatively promoted all renamable local values
(except const non-address taken variables) for both the exporting
and importing module. We would then internalize them back based on
the ThinLink results if they weren't actually exported. This is
inefficient, and results in unnecessary renames. It also meant we
had to check the non-renamability of a value in the summary, which
was already checked during function importing analysis in the ThinLink.

Made renameModuleForThinLTO (which does the promotion/renaming) instead
use the index when exporting, to avoid unnecessary renames/promotions.
For importing modules, we can simply promoted all values as any local
we import by definition is exported and needs promotion.

This required changes to the method used by the FunctionImport pass
(only invoked from 'opt' for testing) and when invoked from llvm-link,
since neither does a ThinLink. We simply conservatively mark all locals
in the index as promoted, which preserves the current aggressive
promotion behavior.

I also needed to change an llvm-lto based test where we had previously
been aggressively promoting values that weren't importable (aliasees),
but now will not promote.

Event Timeline

tejohnson updated this revision to Diff 77385.Nov 9 2016, 1:26 PM
tejohnson retitled this revision from to [ThinLTO] Only promote exported locals as marked in index.
tejohnson updated this object.
tejohnson added a reviewer: mehdi_amini.
tejohnson added a subscriber: llvm-commits.
mehdi_amini added inline comments.Nov 12 2016, 10:20 PM
lib/LTO/ThinLTOCodeGenerator.cpp
554

Not clear why you init TMBuilder, while you only seem to use the Triple?

562

Is it clang-formatted?

lib/Transforms/Utils/FunctionImportUtils.cpp
80

why can't we just promote what is needed?

tejohnson added inline comments.Nov 14 2016, 6:48 AM
lib/LTO/ThinLTOCodeGenerator.cpp
554

Hmm, I cloned this from ThinLTOCodeGenerator::internalize, perhaps it isn't needed in either place. I removed the init from here for now.

562

Yes. I modified the spacing on this line, re-ran clang-format, and got back the same thing.

lib/Transforms/Utils/FunctionImportUtils.cpp
80

See the comment just above this - when we are importing we may not have a summary in the distributed backend case (we only get the summaries for things we want to import as defs, not all references in the module). At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the resulting module.

tejohnson updated this revision to Diff 77813.Nov 14 2016, 7:42 AM

Address comments

mehdi_amini accepted this revision.Nov 14 2016, 8:57 AM
mehdi_amini edited edge metadata.

LGTM.

lib/Transforms/Utils/FunctionImportUtils.cpp
80

See the comment just above this - when we are importing we may not have a summary in the distributed backend case (we only get the summaries for things we want to import as defs, not all references in the module).

That's annoying.

At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the resulting module.

Sure, but I'm still not fan: it means we can't catch error at this stage.

This revision is now accepted and ready to land.Nov 14 2016, 8:57 AM
This revision was automatically updated to reflect the committed changes.

(Hit send too early on previous response)

lib/Transforms/Utils/FunctionImportUtils.cpp
80

At the worst, though, we will simply promote some values that are never referenced after import, so it won't affect the >> resulting module.

Sure, but I'm still not fan: it means we can't catch error at this stage.

Agree, it would be better to be able to handle these based on the index. However, this means including summaries of references (not to be imported as defs) assuming that the backend's invocation of llvm::ComputeCrossModuleImportForModule will always make the same decisions. If someone for example accidentally passes tuning options like -import-instr-limit to the backend and not the thin link this could break things if a superset of things originally decided for import by the thin link are then imported. A more robust solution would be to add another flag to the (combined) index that identifies summaries to import as definitions.