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

Add #[rustc_legacy_const_generics] #82447

Merged
merged 8 commits into from
Feb 25, 2021
Merged

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Feb 23, 2021

This is the first step towards removing #[rustc_args_required_const]: a new attribute is added which rewrites function calls of the form func(a, b, c) to func::<{b}>(a, c). This allows previously stabilized functions in stdarch which use rustc_args_required_const to use const generics instead.

This new attribute is not intended to ever be stabilized, it is only intended for use in stdarch as a replacement for #[rustc_args_required_const].

#[rustc_legacy_const_generics(1)]
pub fn foo<const Y: usize>(x: usize, z: usize) -> [usize; 3] {
    [x, Y, z]
}

fn main() {
    assert_eq!(foo(0 + 0, 1 + 1, 2 + 2), [0, 2, 4]);
    assert_eq!(foo::<{1 + 1}>(0 + 0, 2 + 2), [0, 2, 4]);
}

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 23, 2021
@Amanieu
Copy link
Member Author

Amanieu commented Feb 24, 2021

The current implementation is not very well optimized, let's see how bad it is...

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 24, 2021
@bors
Copy link
Contributor

bors commented Feb 24, 2021

⌛ Trying commit 00eca69 with merge 6d7225329e02f2f6fc73dfb977e802102a412bff...

@bors
Copy link
Contributor

bors commented Feb 24, 2021

☀️ Try build successful - checks-actions
Build commit: 6d7225329e02f2f6fc73dfb977e802102a412bff (6d7225329e02f2f6fc73dfb977e802102a412bff)

@rust-timer
Copy link
Collaborator

Queued 6d7225329e02f2f6fc73dfb977e802102a412bff with parent fe1bf8e, future comparison URL.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I think we should ignore the same-crate situation and just not do anything there. If you can write functions with const generics you can change your call sites, too.

compiler/rustc_ast_lowering/src/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/expr.rs Show resolved Hide resolved
compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (6d7225329e02f2f6fc73dfb977e802102a412bff): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 24, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Feb 24, 2021

Ok, it does have a measurable impact. So... one way to optimize this would be to write the list of all rustc_legacy_const_generics of a crate into metadata. Then, instead of looking up the attribute, we can look into that list (should probably become a hash set upon reading from metadata). Since most crates' list will be entirely empty, this should be really fast to check.

Though... I'm not sure if this is actually a bottleneck at all since there seem to be no additional calls/decodings of item_attrs, though that may just be because these aren't registered yet at these early stages where we don't have tcx yet.

@Amanieu
Copy link
Member Author

Amanieu commented Feb 24, 2021

I think part of the overhead comes from returning a Vec which requires an allocation on every call to item_attrs. I'll try changing that function to take a FnMut(&[Attributes]) closure argument instead, which should avoid the allocation.

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member Author

Amanieu commented Feb 25, 2021

I've addressed the review comments and have attempted to address the performance issues using my favored strategy of throwing a HashMap at the problem.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@Amanieu Amanieu marked this pull request as ready for review February 25, 2021 03:39
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (96bc946ebd85c25f0f05e1b979e8f996be1a1daf): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 25, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 25, 2021

Perf looks much better now :)

@Amanieu
Copy link
Member Author

Amanieu commented Feb 25, 2021

@oli-obk Ready for review.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 25, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2021

📌 Commit 00afbe7 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 Feb 25, 2021
@bors
Copy link
Contributor

bors commented Feb 25, 2021

⌛ Testing commit 00afbe7 with merge 80c5a3d003b0f4610bddd0dd4237980f6f21d9a5...

@bors
Copy link
Contributor

bors commented Feb 25, 2021

💔 Test failed - checks-actions

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@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 Feb 25, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Feb 25, 2021

A lot of builders passed, let's try again

@bors retry

@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 Feb 25, 2021
@bors
Copy link
Contributor

bors commented Feb 25, 2021

⌛ Testing commit 00afbe7 with merge 98f8cce...

@bors
Copy link
Contributor

bors commented Feb 25, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 98f8cce to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 25, 2021
@bors bors merged commit 98f8cce into rust-lang:master Feb 25, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 25, 2021
@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) 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.

10 participants