Skip to content
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

Polymorphization #69749

Merged
merged 13 commits into from
Jul 21, 2020
Merged

Polymorphization #69749

merged 13 commits into from
Jul 21, 2020

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Mar 5, 2020

This PR implements an analysis to detect when functions could remain polymorphic during code generation.

Fixes #46477

r? @eddyb
cc @rust-lang/wg-mir-opt @nikomatsakis @pnkfelix

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2020
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things...

src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
src/librustc_codegen_llvm/callee.rs Outdated Show resolved Hide resolved
src/librustc_mir/monomorphize/polymorphize.rs Outdated Show resolved Hide resolved
src/librustc_mir/monomorphize/polymorphize.rs Outdated Show resolved Hide resolved
Comment on lines 83 to 85
if self.substs.has_param_types() {
if self.substs.has_param_types() && !tcx.sess.opts.debugging_opts.polymorphize {
bug!("Instance.ty called for type {:?} with params in substs: {:?}", ty, self.substs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget where/if I left a comment, but the current situation with Instance's types and Instance::ty_env in particular is pretty bad and we should probably require passing ParamEnv around a lot (or maybe start using ParamEnvAnd<Instance>?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was #67800 (comment).

src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
src/librustc/query/mod.rs Outdated Show resolved Hide resolved
src/librustc/query/mod.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Member

bjorn3 commented Mar 6, 2020

Why does this use ty::Param instead of something like ty::Erased? Is it to reduce the amount of places that you need to change?

@eddyb
Copy link
Member

eddyb commented Mar 6, 2020

Why does this use ty::Param instead of something like ty::Erased? Is it to reduce the amount of places that you need to change?

Using ty::Param should result in more cache reuse, but also it's known to work, since type-checking, miri and other parts of the compiler already extensively rely on it.

Also, if the parameters are truly unused then we might get away without a ParamEnv, but long-term we can rely on ParamEnv to handle parameters used only in certain ways, e.g. #46477 (comment)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 20, 2020
This commit implements the `unused_generic_params` query, an initial
version of polymorphization which detects when an item does not use
generic parameters and is being needlessly monomorphized as a result.

Signed-off-by: David Wood <david@davidtw.co>
This commit normalizes function signatures for instances before
substituting, a workaround for polymorphization considering
parameters unused when they show up in the signature, but not the
body (due to being normalized).

Unfortunately, this causes test output to change with the parallel
compiler only.

Signed-off-by: David Wood <david@davidtw.co>
This commit records the results of `unused_generic_params` in crate
metadata, hopefully improving performance.

Signed-off-by: David Wood <david@davidtw.co>
This commit replaces the `-Z polymorphize-errors` debugging flag with a
`#[rustc_polymorphize_error]` attribute for use on functions.

Signed-off-by: David Wood <david@davidtw.co>
This commit introduces a `FiniteBitSet` type which replaces the manual
bit manipulation which was being performed in polymorphization.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the issue-46477-polymorphization branch from 912db12 to 4b99699 Compare July 20, 2020 18:35
@davidtwco
Copy link
Member Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jul 20, 2020

📌 Commit 4b99699 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 20, 2020

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2020
@bors
Copy link
Contributor

bors commented Jul 21, 2020

⌛ Testing commit 4b99699 with merge b52522a...

@bors
Copy link
Contributor

bors commented Jul 21, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing b52522a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2020
@bors bors merged commit b52522a into rust-lang:master Jul 21, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #69749!

Tested on commit b52522a.
Direct link to PR: #69749

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jul 21, 2020
Tested on commit rust-lang/rust@b52522a.
Direct link to PR: <rust-lang/rust#69749>

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
@davidtwco davidtwco deleted the issue-46477-polymorphization branch July 21, 2020 09:21
bors added a commit to rust-lang/miri that referenced this pull request Jul 23, 2020
rustup

Following rust-lang/rust#69749 I added some `ty::ParamEnv::reveal_all()` even though @eddyb advised me in the past to avoid those.
@nnethercote
Copy link
Contributor

This was a perf loss on landing, as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instantiate fewer copies of a closure inside a generic function