rustc: Prepare to enable ThinLTO by default#46382
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
|
cc @rust-lang/compiler |
|
Is this a blocker? #46346 |
|
Are we sure that's related to ThinLTO? I've tested everything locally and wasn't able to reproduce myself. |
|
Urgh it may very well be. I swear to god. |
kennytm
added
the
S-waiting-on-review
There was a problem hiding this comment.
Why not use a std::unordered_set here.
|
I see that |
|
@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. |
|
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! |
|
Awesome! Will review shortly. Am I reading this right, this is still blocked on #46346? |
|
@michaelwoerister unfortunately yes :( |
michaelwoerister
left a comment
michaelwoerister
left a comment
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
compilation compilation
There was a problem hiding this comment.
Was this too conservative before?
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 rust-lang#46374
Helps to avoid hitting path limits on Windows
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
force-pushed
the
thinlto-default
branch
from
e527fa6 to
855f6d1
Compare
alexcrichton
changed the title
|
@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
|
|
Thanks for the explanation! @bors r+ |
|
📌 Commit 855f6d1 has been approved by |
kennytm
added
S-waiting-on-bors
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.
|
☀️ Test successful - status-appveyor, status-travis |
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
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
8 participants
This commit almost enables ThinLTO and multiple codegen units in release mode by
default but is blocked on #46346 now before pulling the trigger.