Skip to content

Polymorphization#69749

Merged
bors merged 13 commits intorust-lang:masterfrom
davidtwco:issue-46477-polymorphization
Jul 21, 2020
Merged

Polymorphization#69749
bors merged 13 commits intorust-lang:masterfrom
davidtwco:issue-46477-polymorphization

Conversation

@davidtwco
Copy link
Member

@davidtwco davidtwco commented Mar 5, 2020 •
edited
Loading

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.

Sign in to view
Centril
Centril reviewed Mar 6, 2020
View reviewed changes
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...

eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
src/librustc/ty/instance.rs Outdated
Comment on lines 83 to 85
Copy link
Member

@eddyb eddyb Mar 6, 2020

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

@eddyb eddyb Mar 6, 2020

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).

eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
@davidtwco davidtwco mentioned this pull request Mar 6, 2020
Closed
@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)

eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
eddyb
eddyb reviewed Mar 6, 2020
View reviewed changes
@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
Collaborator

bors commented Jul 20, 2020

⌛ Testing commit 912db12b2987099ce7f0a3826106b92cf8e0cbde with merge b40298da8de398d2a20f6d0ec3ea9cb262a205c0...

@bors

This comment has been minimized.

Sign in to view
@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
davidtwco added 5 commits July 20, 2020 19:35
@davidtwco
mir: unused_generic_params query
2989fea 
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>
@davidtwco
ty: normalize fn sigs before subst
f52c729 
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>
@davidtwco
metadata: record unused_generic_params
5ce29d3 
This commit records the results of `unused_generic_params` in crate
metadata, hopefully improving performance.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
mir: use attribute over -Z polymorphize-errors
b1f8bd6 
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>
@davidtwco
index: introduce and use FiniteBitSet
4b99699 
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

davidtwco commented Jul 20, 2020

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Jul 20, 2020

📌 Commit 4b99699 has been approved by eddyb

@bors
Copy link
Collaborator

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
Collaborator

bors commented Jul 21, 2020

⌛ Testing commit 4b99699 with merge b52522a...

@bors
Copy link
Collaborator

bors commented Jul 21, 2020

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

@rust-highfive rust-highfive mentioned this pull request Jul 21, 2020
Closed
@rust-highfive
Copy link
Contributor

rust-highfive commented Jul 21, 2020

📣 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).

This was referenced Jul 21, 2020
Closed
Closed
@JohnTitor JohnTitor mentioned this pull request Jul 22, 2020
Closed
This was referenced Jul 22, 2020
Closed
Merged
@nnethercote
Copy link
Contributor

nnethercote commented Jul 27, 2020

This was a perf loss on landing, as expected.

@nnethercote nnethercote mentioned this pull request Jul 27, 2020
Merged
@RalfJung RalfJung mentioned this pull request May 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nnethercote nnethercote nnethercote left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@lcnr lcnr lcnr left review comments

@nikomatsakis nikomatsakis Awaiting requested review from nikomatsakis

+6 more reviewers

@Centril Centril Centril left review comments

@jplatte jplatte jplatte left review comments

@bluss bluss bluss left review comments

@ljedrz ljedrz ljedrz left review comments

@varkor varkor varkor left review comments

@eddyb eddyb eddyb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@eddyb eddyb

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Instantiate fewer copies of a closure inside a generic function