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

Permit asm_const and asm_sym to reference generic params #96800

Merged
merged 4 commits into from
May 18, 2022

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented May 7, 2022

Related #96557

These constructs will be allowed:

fn foofoo<const N: usize>() {}

unsafe fn foo<const N: usize>() {
    asm!("/* {0} */", const N);
    asm!("/* {0} */", const N + 1);
    asm!("/* {0} */", sym foofoo::<N>);
}

fn barbar<T>() {}

unsafe fn bar<T>() {
    asm!("/* {0} */", const std::mem::size_of::<T>());
    asm!("/* {0} */", const std::mem::size_of::<(T, T)>());
    asm!("/* {0} */", sym barbar::<T>);
    asm!("/* {0} */", sym barbar::<(T, T)>);
}

@Amanieu, I didn't switch inline asms to use DefKind::InlineAsm, as I see little value doing that; given that no type inference is needed, it will only make typecking slower and more complex but will have no real gains. I did switch them to follow the same code path as inline asm during symbol resolution, though.
The error: unconstrained generic constant you mentioned in #76001 is due to the fact that to_const will actually add a wfness obligation to the constant, which we don't need for asm_const, so I have that removed.

@rustbot label: +A-inline-assembly +F-asm

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) labels May 7, 2022
@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2022
@Amanieu
Copy link
Member

Amanieu commented May 7, 2022

LGTM!

asm!("/* {0} */", const std::mem::size_of::<T>());
asm!("/* {0} */", const std::mem::size_of::<(T, T)>());
asm!("/* {0} */", sym barbar::<T>);
asm!("/* {0} */", sym barbar::<(T, T)>);
Copy link
Member

Choose a reason for hiding this comment

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

We are missing a feature gate test for these constructs with generics I think.

@nagisa
Copy link
Member

nagisa commented May 10, 2022

r=me once asm-const and asm-sym feature gate tests are augmented.

@Amanieu
Copy link
Member

Amanieu commented May 10, 2022

sym and const operands are still unstable, is it necessary to add a separate feature gate for their use with generics?

@nagisa
Copy link
Member

nagisa commented May 10, 2022

I don't think its necessary to add an entirely separate test, but I think augmenting the existing feature-gate-asm_sym and feature-gate-asm_const with just one additional line each is probably worthwhile, if only to serve as a reminder when we look back at stabilizing these features.

@nagisa
Copy link
Member

nagisa commented May 18, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2022

📌 Commit 441d98f has been approved by nagisa

@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 May 18, 2022
@bors
Copy link
Contributor

bors commented May 18, 2022

⌛ Testing commit 441d98f with merge 10d9ecd...

@bors
Copy link
Contributor

bors commented May 18, 2022

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 10d9ecd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2022
@bors bors merged commit 10d9ecd into rust-lang:master May 18, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 18, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (10d9ecd): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) 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.

7 participants