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

Optimize const value interning for ZST types #78061

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

wesleywiser
Copy link
Member

Interning can skip any inhabited ZST type in general.

Fixes #68010

r? @oli-obk

Interning can skip any inhabited ZST type in general.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2020
@wesleywiser wesleywiser added the I-compiletime Issue: Problems and improvements with respect to compile times. label Oct 17, 2020
@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 17, 2020

⌛ Trying commit 1d07d69 with merge 25cf147f8ee183a6b207e49e0339d8189e8e9280...

@bors
Copy link
Contributor

bors commented Oct 17, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 25cf147f8ee183a6b207e49e0339d8189e8e9280 (25cf147f8ee183a6b207e49e0339d8189e8e9280)

@rust-timer
Copy link
Collaborator

Queued 25cf147f8ee183a6b207e49e0339d8189e8e9280 with parent ffeeb20, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (25cf147f8ee183a6b207e49e0339d8189e8e9280): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 20, 2020

📌 Commit 1d07d69 has been approved by oli-obk

@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 Oct 20, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

Can you check whether this also fixes #77062?

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 20, 2020
…, r=oli-obk

Optimize const value interning for ZST types

Interning can skip any inhabited ZST type in general.

Fixes rust-lang#68010

r? @oli-obk
This was referenced Oct 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2020
…laumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#78046 (Add codegen test for issue rust-lang#73827)
 - rust-lang#78061 (Optimize const value interning for ZST types)
 - rust-lang#78070 (we can test std and core panic macros together)
 - rust-lang#78076 (Move orphan module-name/mod.rs files into module-name.rs files)
 - rust-lang#78129 (Wrapping intrinsics doc links update.)
 - rust-lang#78133 (Add some MIR-related regression tests)
 - rust-lang#78144 (Don't update `entries` in `TypedArena` if T does not need drop)
 - rust-lang#78145 (Drop unneeded `mut`)
 - rust-lang#78157 (Remove unused type from librustdoc)

Failed merges:

r? `@ghost`
@bors bors merged commit 3fea201 into rust-lang:master Oct 20, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 20, 2020
@wesleywiser
Copy link
Member Author

Good call! That's fixed too. I'll open another PR with the test case added.

@@ -187,6 +187,12 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
return walked;
}
}

// ZSTs do not need validation unless they're uninhabited
Copy link
Member

Choose a reason for hiding this comment

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

This is not validation, this is interning, so I find the comment here rather confusing.

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 actually we can skip even uninhabited ZST, precisely because this is not validation but just looking for pointers: #78179.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large ZST arrays take a looong time to compile
7 participants