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.

Sign up for GitHub

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

Jump to bottom

Fix several asm! related issues #94169

Merged
merged 5 commits into from
Feb 22, 2022
Merged

Fix several asm! related issues #94169

merged 5 commits into from
Feb 22, 2022

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Feb 19, 2022

This is a combination of several fixes, each split into a separate commit. Splitting these into PRs is not practical since they conflict with each other.

Fixes #92378
Fixes #85247

r? @nagisa

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 19, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
nagisa
nagisa reviewed Feb 19, 2022
View reviewed changes
compiler/rustc_typeck/src/collect.rs
/// inline assembly.
fn asm_target_features<'tcx>(tcx: TyCtxt<'tcx>, id: DefId) -> &'tcx FxHashSet<Symbol> {
let mut target_features = tcx.sess.target_features.clone();
let attrs = tcx.codegen_fn_attrs(id);
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 one problem that this has is that -Ctarget-feature flag attributes are not accounted for here, to the best of my knowledge. This is something we should eventually fix (at the very least because #[cfg(target_feature=...)] honour the global flags.) One of the commits in #87402 gets us partway towards a solution there by exposing the globally enabled features as a query, but those are still backend-specific names so not applicable for assembly quite yet.

@nagisa
Copy link
Member

nagisa commented Feb 19, 2022

Overall seems like an improvement. I'm happy to r+ this with or without the 2nd comment being addressed.

The 1st comment is mostly just an observation and something to keep in mind, as it is not necessarily something we can easily fix right now anyway.

@nagisa
Copy link
Member

nagisa commented Feb 20, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2022

📌 Commit f10f921 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 Feb 20, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 20, 2022
@matthiaskrgr
Rollup merge of rust-lang#94169 - Amanieu:asm_stuff, r=nagisa
fd448d5 
Fix several asm! related issues

This is a combination of several fixes, each split into a separate commit. Splitting these into PRs is not practical since they conflict with each other.

Fixes rust-lang#92378
Fixes rust-lang#85247

r? `@nagisa`
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 20, 2022
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 20, 2022
@Manishearth
Rollup merge of rust-lang#94169 - Amanieu:asm_stuff, r=nagisa
97abac2 
Fix several asm! related issues

This is a combination of several fixes, each split into a separate commit. Splitting these into PRs is not practical since they conflict with each other.

Fixes rust-lang#92378
Fixes rust-lang#85247

r? ``@nagisa``
@Manishearth Manishearth mentioned this pull request Feb 20, 2022
Closed
@Manishearth
Copy link
Member 

tidy error: /checkout/compiler/rustc_typeck/src/collect.rs: too many lines (3010) (add `// ignore-tidy-filelength` to the file to suppress this error)

https://github.com/rust-lang-ci/rust/runs/5267509172?check_suite_focus=true

@Manishearth
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 21, 2022
@Amanieu
Simplify gating of BPF w registers behind the alu32 target feature
2e8a766 
This is already handled by supported_types().
@Amanieu
On ARM, use relocation_model to detect whether r9 should be reserved
1ceb104 
The previous approach of checking for the reserve-r9 target feature
didn't actually work because LLVM only sets this feature very late when
initializing the per-function subtarget.
@Amanieu
Take CodegenFnAttrs into account when validating asm! register operands
fc41d4b 
Checking of asm! register operands now properly takes function
attributes such as #[target_feature] and #[instruction_set] into
account.
@Amanieu
Add tests
fb5539b
@Amanieu
Copy link
Member Author

Amanieu commented Feb 21, 2022

I'm not sure how I should deal with this, should I just add // ignore-tidy-filelength to collect.rs?

@nagisa
Copy link
Member

nagisa commented Feb 21, 2022

The alternative would be to move the query implementation to somewhere else (maybe rustc_passes::intrinsicck which also provides a query implementation?).

But yeah, adding the comment is something you can do. In fact this comment was present in this file before bb0a2f9.

@Amanieu
Copy link
Member Author

Amanieu commented Feb 21, 2022

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Feb 21, 2022

📌 Commit a60b791 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 21, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#94169 - Amanieu:asm_stuff, r=nagisa
fa60d8d 
Fix several asm! related issues

This is a combination of several fixes, each split into a separate commit. Splitting these into PRs is not practical since they conflict with each other.

Fixes rust-lang#92378
Fixes rust-lang#85247

r? `@nagisa`
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 22, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2022
@bors
Auto merge of rust-lang#94254 - matthiaskrgr:rollup-7llbjhd, r=matthi…
68369a0 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#94169 (Fix several asm! related issues)
 - rust-lang#94178 (tidy: fire less "ignoring file length unneccessarily" warnings)
 - rust-lang#94179 (solarish current_exe using libc call directly)
 - rust-lang#94196 (compiletest: Print process output info with less whitespace)
 - rust-lang#94208 (Add the let else tests found missing in the stabilization report)
 - rust-lang#94237 (Do not suggest wrapping an item if it has ambiguous un-imported methods)
 - rust-lang#94246 (ScalarMaybeUninit is explicitly hexadecimal in its formatting)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1cf2e69 into rust-lang:master Feb 22, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 22, 2022
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 23, 2022
@matthiaskrgr
Rollup merge of rust-lang#94169 - Amanieu:asm_stuff, r=nagisa
a063e13 
Fix several asm! related issues

This is a combination of several fixes, each split into a separate commit. Splitting these into PRs is not practical since they conflict with each other.

Fixes rust-lang#92378
Fixes rust-lang#85247

r? ``@nagisa``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

Assignees

@nagisa nagisa

Labels
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
1.61.0
7 participants
@Amanieu @rust-highfive @rust-log-analyzer @nagisa @bors @Manishearth @rustbot