New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc: Prepare to enable ThinLTO by default #46382

Merged
merged 3 commits into from Dec 3, 2017

Conversation

Projects
None yet
8 participants
@alexcrichton
Member

alexcrichton commented Nov 29, 2017

This commit almost enables ThinLTO and multiple codegen units in release mode by
default but is blocked on #46346 now before pulling the trigger.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 29, 2017

Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Nov 29, 2017

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented Nov 29, 2017

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Nov 29, 2017

Member

Is this a blocker? #46346

Member

sfackler commented Nov 29, 2017

Is this a blocker? #46346

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 29, 2017

Member

Are we sure that's related to ThinLTO? I've tested everything locally and wasn't able to reproduce myself.

Member

alexcrichton commented Nov 29, 2017

Are we sure that's related to ThinLTO? I've tested everything locally and wasn't able to reproduce myself.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 29, 2017

Member

Urgh it may very well be. I swear to god.

Member

alexcrichton commented Nov 29, 2017

Urgh it may very well be. I swear to god.

// summaries in the index, and we basically just only want to ensure that dead
// symbols are internalized. Otherwise everything that's already external
// linkage will stay as external, and internal will stay as internal.
std::set<GlobalValue::GUID> ExportedGUIDs;

This comment has been minimized.

@kennytm

kennytm Nov 29, 2017

Member

Why not use a std::unordered_set here.

@kennytm

kennytm Nov 29, 2017

Member

Why not use a std::unordered_set here.

@cuviper

This comment has been minimized.

Show comment
Hide comment
@cuviper

cuviper Nov 30, 2017

Member

I see that LLVMRustThinLTOAvailable gates on just LLVM_VERSION_GE(4, 0). How well are you expecting this to work when using an external LLVM-4.0?
(IOW, are there rust-lang/llvm patches that I definitely need here?)

Member

cuviper commented Nov 30, 2017

I see that LLVMRustThinLTOAvailable gates on just LLVM_VERSION_GE(4, 0). How well are you expecting this to work when using an external LLVM-4.0?
(IOW, are there rust-lang/llvm patches that I definitely need here?)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 30, 2017

Member

@cuviper IIRC rust-lang/llvm@e45c75d is the main (and possibly only) ThinLTO related patch (although it fixes a preexisting bug).

In general though I lose track over time what patch was for what on our fork, although everything that matters is upstream in some form so it's just a matter of time until we update to get it.

Member

alexcrichton commented Nov 30, 2017

@cuviper IIRC rust-lang/llvm@e45c75d is the main (and possibly only) ThinLTO related patch (although it fixes a preexisting bug).

In general though I lose track over time what patch was for what on our fork, although everything that matters is upstream in some form so it's just a matter of time until we update to get it.

@cuviper

This comment has been minimized.

Show comment
Hide comment
@cuviper

cuviper Nov 30, 2017

Member

Ok, thanks, we already grabbed that one. I'll look back over the branch again later myself. I do appreciate that everything is fixed upstream first!

Member

cuviper commented Nov 30, 2017

Ok, thanks, we already grabbed that one. I'll look back over the branch again later myself. I do appreciate that everything is fixed upstream first!

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Nov 30, 2017

Contributor

Awesome! Will review shortly. Am I reading this right, this is still blocked on #46346?

Contributor

michaelwoerister commented Nov 30, 2017

Awesome! Will review shortly. Am I reading this right, this is still blocked on #46346?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 30, 2017

Member

@michaelwoerister unfortunately yes :(

Member

alexcrichton commented Nov 30, 2017

@michaelwoerister unfortunately yes :(

@michaelwoerister

The PR looks good to me. Let's hope it doesn't bit rot too much until #46346 is resolved.

Can you explain to me why you don't use addPreservedGUID anymore?

Show outdated Hide outdated src/librustc/session/mod.rs Outdated
Show outdated Hide outdated src/libstd/sys_common/backtrace.rs Outdated
for (int i = 0; i < num_symbols; i++) {
addPreservedGUID(Ret->Index,
Ret->GUIDPreservedSymbols,
GlobalValue::getGUID(preserved_symbols[i]));

This comment has been minimized.

@michaelwoerister

michaelwoerister Nov 30, 2017

Contributor

Was this too conservative before?

@michaelwoerister

michaelwoerister Nov 30, 2017

Contributor

Was this too conservative before?

alexcrichton added some commits Nov 29, 2017

rustc: Tweak the `isExported` callback for ThinLTO
Previously we were too eagerly exporting almost all symbols used in ThinLTO
which can cause a whole host of problems downstream! This commit instead fixes
this error by aligning more closely with `lib/LTO/LTO.cpp` in LLVM's codebase
which is to only change the linkage of summaries which are computed as dead.

Closes #46374
Rename long test names to be a bit shorter
Helps to avoid hitting path limits on Windows
rustc: Prepare to enable ThinLTO by default
This commit prepares to enable ThinLTO and multiple codegen units in release
mode by default. We've still got a debuginfo bug or two to sort out before
actually turning it on by default.

@alexcrichton alexcrichton changed the title from rustc: Enable ThinLTO by default to rustc: Prepare to enable ThinLTO by default Nov 30, 2017

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 30, 2017

Member

@michaelwoerister ok I've updated the listing of commits and addressed comments. The commits now to not enable ThinLTO by default but instead retain retain today's default. The repo is left in a state though to make it a one line change to turn ThinLTO on by default in the future.

To answer your question about GUIDPreservedSymbols though there's more info on #46374 but it boils down to two things:

  • In the first location where GUIDPreservedSymbols was used the logic of addPreservedGUID wasn't necessary. The same result is computed whether we call addPreservedGUID or not.
  • In the second location it was incorrect to call addPreservedGUID in the sense that it added the transitive closure of symbols to that set. That in turn caused #46374 where too many symbols were exported (aka all of them) rather than just the set we wanted. By avoiding addPreservedGUID and tweaking the definition of isExported this bug was fixed though.
Member

alexcrichton commented Nov 30, 2017

@michaelwoerister ok I've updated the listing of commits and addressed comments. The commits now to not enable ThinLTO by default but instead retain retain today's default. The repo is left in a state though to make it a one line change to turn ThinLTO on by default in the future.

To answer your question about GUIDPreservedSymbols though there's more info on #46374 but it boils down to two things:

  • In the first location where GUIDPreservedSymbols was used the logic of addPreservedGUID wasn't necessary. The same result is computed whether we call addPreservedGUID or not.
  • In the second location it was incorrect to call addPreservedGUID in the sense that it added the transitive closure of symbols to that set. That in turn caused #46374 where too many symbols were exported (aka all of them) rather than just the set we wanted. By avoiding addPreservedGUID and tweaking the definition of isExported this bug was fixed though.
@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Nov 30, 2017

Contributor

Thanks for the explanation!

@bors r+

Contributor

michaelwoerister commented Nov 30, 2017

Thanks for the explanation!

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 30, 2017

Contributor

📌 Commit 855f6d1 has been approved by michaelwoerister

Contributor

bors commented Nov 30, 2017

📌 Commit 855f6d1 has been approved by michaelwoerister

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 2, 2017

Contributor

⌛️ Testing commit 855f6d1 with merge 088f328...

Contributor

bors commented Dec 2, 2017

⌛️ Testing commit 855f6d1 with merge 088f328...

bors added a commit that referenced this pull request Dec 2, 2017

Auto merge of #46382 - alexcrichton:thinlto-default, r=michaelwoerister
rustc: Prepare to enable ThinLTO by default

This commit *almost* enables ThinLTO and multiple codegen units in release mode by
default but is blocked on #46346 now before pulling the trigger.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 3, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 088f328 to master...

Contributor

bors commented Dec 3, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 088f328 to master...

@bors bors merged commit 855f6d1 into rust-lang:master Dec 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 7, 2017

rustc: Further tweak linkage in ThinLTO
In rust-lang#46382 the logic around linkage preservation with ThinLTO ws tweaked but the
loop that registered all otherwise exported GUID values as "don't internalize
me please" was erroneously too conservative and only asking "external" linkage
items to not be internalized. Instead we actually want the inversion of that
condition, everything *without* "local" linkage to be internalized.

This commit updates the condition there, adds a test, and...

Closes rust-lang#46543

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 7, 2017

rustc: Further tweak linkage in ThinLTO
In rust-lang#46382 the logic around linkage preservation with ThinLTO ws tweaked but the
loop that registered all otherwise exported GUID values as "don't internalize
me please" was erroneously too conservative and only asking "external" linkage
items to not be internalized. Instead we actually want the inversion of that
condition, everything *without* "local" linkage to be internalized.

This commit updates the condition there, adds a test, and...

Closes rust-lang#46543

bors added a commit that referenced this pull request Dec 8, 2017

Auto merge of #46549 - alexcrichton:thinlto-weak, r=michaelwoerister
rustc: Further tweak linkage in ThinLTO

In #46382 the logic around linkage preservation with ThinLTO ws tweaked but the
loop that registered all otherwise exported GUID values as "don't internalize
me please" was erroneously too conservative and only asking "external" linkage
items to not be internalized. Instead we actually want the inversion of that
condition, everything *without* "local" linkage to be internalized.

This commit updates the condition there, adds a test, and...

Closes #46543

@alexcrichton alexcrichton referenced this pull request Feb 1, 2018

Closed

LTO: 10x regression #47866

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment