[WIP] Deduplicate instances #48139
[WIP] Deduplicate instances #48139
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
pietroalbini
added
the
S-waiting-on-review
|
@bjorn3 can you give me a bit more context of what this PR is trying to achieve? |
|
This is trying to prevent duplicate instantiations of generic functions when there are generics which dont matter for codegen. For example: fn abc<T>() {
println!("djfjfjfb");
}
fn main() {
abc::<()>();
abc::<u8>();
}This used to instantiate both abc::<()> and abc::() before this pr and just abc::<{some replacement}> with this pr. |
|
@bjorn3 I see! Thanks for the context. Is your plan to detect only those cases that don't affect codegen at all? This feels like a non-trivial change, but one that I would love to see. Complex enough though it's worth describing your strategy a bit up front. (I'd like to eventually extend it -- e.g., so that we can detect cases where the only thing that matters about a type is its size and collapse those instantiations, etc.) |
Yes, i am visiting the mir of a fn and storing all used TyParam's. During collection of trans items and translating calls all every subst which has no associated TyParam in their mir gets replaced by |
src/librustc/ty/mod.rs
Outdated
| @@ -2762,3 +2762,51 @@ impl fmt::Display for SymbolName { | |||
| fmt::Display::fmt(&self.name, fmt) | |||
| } | |||
| } | |||
|
|
|||
| pub fn ty_fn_sig<'a, 'tcx: 'a>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
There was a problem hiding this comment.
Maybe combine this with the existing fn_sig helper?
There was a problem hiding this comment.
I just moved it from librustc_trans/common.rs. It also uses fn_sig internally.
There was a problem hiding this comment.
What I meant is that maybe we should make it part of fn_sig instead, so that you just write tcx.fn_sig(...) and not ty_fn_sig(tcx, ...).
| use rustc::mir::{Mir, Rvalue, Promoted, Location}; | ||
| use rustc::mir::visit::{Visitor, TyContext}; | ||
|
|
||
| /// Replace substs which arent used by the function with TyError, |
| use rustc::mir::visit::{Visitor, TyContext}; | ||
|
|
||
| /// Replace substs which arent used by the function with TyError, | ||
| /// so that it doesnt end up in the binary multiple times |
There was a problem hiding this comment.
Nit: s/doesnt/doesn't/
|
|
||
| /// Replace substs which arent used by the function with TyError, | ||
| /// so that it doesnt end up in the binary multiple times | ||
| pub(crate) fn collapse_interchangable_instances<'a, 'tcx>( |
There was a problem hiding this comment.
I'm glad you wrote a comment! But I think we could and should ultimately say more. I'd love to show a (small) example Rust program that would benefit, and then explain (in terms of that example) what this query inputs and outputs exactly.
e.g., some program like
fn foo<T>() { } // here, T is clearly unused =)
fn main() {
foo::<u32>();
foo::<u64>();
}| /// so that it doesnt end up in the binary multiple times | ||
| pub(crate) fn collapse_interchangable_instances<'a, 'tcx>( | ||
| tcx: TyCtxt<'a, 'tcx, 'tcx>, | ||
| mut inst: Instance<'tcx> |
There was a problem hiding this comment.
can we write "instance"? I find inst rather ambiguous ("instruction?")
| } | ||
|
|
||
| let used_substs = used_substs_for_instance(tcx, inst); | ||
| inst.substs = tcx._intern_substs(&inst.substs.into_iter().enumerate().map(|(i, subst)| { |
There was a problem hiding this comment.
seems bad that this function's name is _intern_substs. Seems like you're not suposed to be calling it =)
| } | ||
|
|
||
| #[derive(Debug, Default, Clone)] | ||
| pub struct UsedSubsts { |
There was a problem hiding this comment.
I don't think we should call this UsedSubsts, but rather UsedParameters -- I think of substs as being the set of values that we use to substitute in. e.g., if you have struct Vec<T>, the T is not a subst but rather a generic parameter. In contrast, the u32 in Vec<u32> is a subst.
| let used_substs = used_substs_for_instance(tcx, inst); | ||
| inst.substs = tcx._intern_substs(&inst.substs.into_iter().enumerate().map(|(i, subst)| { | ||
| if let Some(ty) = subst.as_type() { | ||
| let ty = if used_substs.substs.iter().find(|p|p.idx == i as u32).is_some() { |
There was a problem hiding this comment.
Can we add some helper methods here to make this more readable?
e.g.,
if used_substs.param_is_used(p) {
ty
} else {
...
}
| let ty = if used_substs.substs.iter().find(|p|p.idx == i as u32).is_some() { | ||
| ty.into() | ||
| } else if let ty::TyParam(ref _param) = ty.sty { | ||
| //^ Dont replace <closure_kind> and other internal params |
There was a problem hiding this comment.
aren't we bailing earlier for closures?
If not, it feels like the used_substs thing should just return a value that ensures that closure-kind and other parameters would be considered used, rather than special casing on this side.
There was a problem hiding this comment.
couldnt get that working :(
| tcx.mk_ty(ty::TyNever) | ||
| } | ||
| } else { | ||
| // Can't use TyError as it gives some ICE in rustc_trans::callee::get_fn |
There was a problem hiding this comment.
Right, TyError is meant to be used when a compilation error has already been reported (that has not happened here). It's important we maintain that invariant or else legit errors may get suppressed.
|
cc @eddyb |
|
If we can't use |
|
What |
bjorn3
force-pushed
the
deduplicate_instances
branch
4 times, most recently
from
7cb418c
to
5b16548
Compare
|
@bjorn3 If you put the So e.g. you start with |
|
@eddyb I get what you mean, but I think we need a custom type for params which are only dependent on size and align as @Manishearth wants anyway.
|
pietroalbini
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #48208) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@bjorn3 That's kind of hard, I wonder if @Manishearth meant "types which don't have a size that depends on type parameters" (e.g. |
bjorn3
force-pushed
the
deduplicate_instances
branch
from
5b16548
to
89eeacc
Compare
|
I don't recall ever saying that -- got a link? I probably did, but I've completely forgotten the context 😄 |
|
@Manishearth |
|
☔ The latest upstream changes (presumably #46882) made this pull request unmergeable. Please resolve the merge conflicts. |
bjorn3
force-pushed
the
deduplicate_instances
branch
from
89eeacc
to
f376ebc
Compare
|
☔ The latest upstream changes (presumably #48411) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc/ich/impls_ty.rs
Outdated
| @@ -819,7 +819,8 @@ for ty::TypeVariants<'gcx> | |||
| TyChar | | |||
| TyStr | | |||
| TyError | | |||
| TyNever => { | |||
| TyNever | | |||
| TyUnusedParam | ty::TyLayoutOnlyParam(_, _) => { | |||
src/librustc/traits/coherence.rs
Outdated
| @@ -482,7 +482,8 @@ fn ty_is_local_constructor(ty: Ty, in_crate: InCrate) -> bool { | |||
| ty::TyClosure(..) | | |||
| ty::TyGenerator(..) | | |||
| ty::TyGeneratorWitness(..) | | |||
| ty::TyAnon(..) => { | |||
| ty::TyAnon(..) | | |||
| ty::TyUnusedParam | ty::TyLayoutOnlyParam(_, _) => { | |||
|
Still unsure of the status of this PR -- but I'm excited to see continued work on it! 💯 Just let me know when you want me to do something. =) |
bjorn3
force-pushed
the
deduplicate_instances
branch
from
a9aef76
to
7a158b0
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.If this was not helpful or you have suggestes for improvements, please ping or otherwise contact |
|
cc @rust-lang/wg-codegen |
|
☔ The latest upstream changes (presumably #49661) made this pull request unmergeable. Please resolve the merge conflicts. |
bjorn3
force-pushed
the
deduplicate_instances
branch
from
7a158b0
to
bbe99cc
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bjorn3
force-pushed
the
deduplicate_instances
branch
from
bbe99cc
to
7a158b0
Compare
|
Ping from triage @bjorn3! What's the status of this PR? |
|
Some tests are failing and it needs rebase. |
pietroalbini
added
the
T-compiler
|
Weekly ping from triage @bjorn3 :) |
|
Nothing changed and I havent got time yet. |
|
Don't worry @bjorn3! I'm closing the PR to keep the queue tidy, but that doesn't mean the PR is rejected: when you get some free time please reopen it, and we will happily review it! |
pietroalbini
added
S-inactive
|
No problem |
Fixes #46477
TODO: