-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
trans: Internalize symbols without relying on LLVM #43183
trans: Internalize symbols without relying on LLVM #43183
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
@@ -28,59 +27,87 @@ pub enum SymbolExportLevel { | |||
} | |||
|
|||
/// The set of symbols exported from each crate in the crate graph. | |||
#[derive(Clone, Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be Clone
? It looks like it's a fairly large structure that ideally we wouldn't clone. It seems like it's in an Arc most of the time anyway so this feels unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed not necessary anymore. Will remove.
src/librustc_trans/collector.rs
Outdated
{ | ||
assert!(!self.index.contains_key(&source)); | ||
|
||
let start_index = self.targets.len(); | ||
self.targets.extend(targets); | ||
let (targets_size_hint, targets_size_hint_max) = targets.size_hint(); | ||
debug_assert_eq!(targets_size_hint_max, Some(targets_size_hint)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like what we really want here is an ExactSizeIterator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it.
src/librustc_trans/partitioning.rs
Outdated
cgu_name: cgu.name.clone() | ||
}; | ||
|
||
'item: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to no longer be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. It's used in the nested continue
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably just missing something, but I don't see a nested loop above it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, the only place those nested loops still exist is in my head :)
Sorry for the confusion. I'll remove the label.
src/librustc_trans/partitioning.rs
Outdated
// Some accessors might not have been | ||
// instantiated. We can safely ignore those. | ||
trans_item_placements.get(accessor) | ||
.cloned() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to clone the option? It looks like that shouldn't be necessary -- we only use it for PartialEq
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cloned()
(as opposed to clone()
) turns the Option<&T>
into an Option<T>
, which is what we need for filter_map
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you meant now. Yes, cloning seems unnecessary.
src/librustc_trans/collector.rs
Outdated
hir::ItemFn(.., ref generics, _) => { | ||
if !generics.is_type_parameterized() { | ||
hir::ItemFn(_, _, constness, _, ref generics, _) => { | ||
let is_const = match constness { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend tcx.is_const_fn(def_id)
- similarly for hir::Generics
, ty::Generics
should be preferred, although that one is preexisting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does is_const_fn
do something differently than the code below? I'm not against changing, but the constness
and generics
from the HIR item are right there without the need for a hashtable lookup.
src/librustc_trans/collector.rs
Outdated
@@ -889,7 +956,14 @@ impl<'b, 'a, 'v> ItemLikeVisitor<'v> for RootCollector<'b, 'a, 'v> { | |||
} | |||
}; | |||
|
|||
if !generics.is_type_parameterized() && !is_impl_generic { | |||
let is_const = match constness { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
r=me modulo nits and the build failure |
ad74e77
to
2b9ab65
Compare
2b9ab65
to
2f07eb3
Compare
efdc922
to
c93e62b
Compare
@bors r=eddyb All nits addressed, I think. |
📌 Commit 678d377 has been approved by |
⌛ Testing commit 678d377 with merge 685ccdf38707dc948ec9bbde82caa8c9e0ac5cdc... |
💔 Test failed - status-travis |
|
@bors r=eddyb That assertion should be fixed, hopefully. |
📌 Commit 226bc92 has been approved by |
⌛ Testing commit 226bc92 with merge 82e2f9ddd4d0d32bb1ab96928e099c49975d932c... |
💔 Test failed - status-travis |
@bors retry |
⌛ Testing commit 226bc92 with merge b45d8f0ebcf760610826ce2a43f5f3d437916086... |
💔 Test failed - status-appveyor |
Spurious. Could this be related to https://appveyor.statuspage.io/incidents/m2vdvw39kdk8?
|
@bors retry (No harm in retrying while the queue is empty, I guess) |
⌛ Testing commit 226bc92 with merge 49765af5c6bcd1182b8d279b404d79fdd8a81c18... |
💔 Test failed - status-appveyor |
@bors: retry
…On Wed, Jul 19, 2017 at 6:59 AM, bors ***@***.***> wrote:
💔 Test failed - status-appveyor
<https://ci.appveyor.com/project/rust-lang/rust/build/1.0.3983>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#43183 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AaPN0DaQBIlhKb8psY9iNfX3uCfRj9k0ks5sPe-hgaJpZM4OV07r>
.
--
You received this message because you are subscribed to the Google Groups
"rust-ops" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to ***@***.***
To view this discussion on the web visit https://groups.google.com/d/
msgid/rust-ops/rust-lang/rust/pull/43183/c316363043%40github.com
<https://groups.google.com/d/msgid/rust-ops/rust-lang/rust/pull/43183/c316363043%40github.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
|
@nagbot-rs: 🔑 Insufficient privileges: and not in try users |
@bors: retry |
⌛ Testing commit 226bc92 with merge 528330520a5ad6953e4a0f25f79d4db1ad019e7f... |
💔 Test failed - status-appveyor |
|
Thanks, @kennytm! |
(and a few more) |
42c4564
to
e8ff75b
Compare
e8ff75b
to
f6e5416
Compare
📌 Commit f6e5416 has been approved by |
…lvm, r=eddyb trans: Internalize symbols without relying on LLVM This PR makes the compiler use the information gather by the trans collector in order to determine which symbols/trans-items can be made internal. This has the advantages: + of being LLVM independent, + of also working in incremental mode, and + of allowing to not keep all LLVM modules in memory at the same time. This is in preparation for fixing issue #39280. cc @rust-lang/compiler
☀️ Test successful - status-appveyor, status-travis |
This PR makes the compiler use the information gather by the trans collector in order to determine which symbols/trans-items can be made internal. This has the advantages:
This is in preparation for fixing issue #39280.
cc @rust-lang/compiler