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

asm: Allow the use of r8-r14 as clobbers on Thumb1 #93877

Merged
merged 1 commit into from
Feb 19, 2022
Merged

asm: Allow the use of r8-r14 as clobbers on Thumb1 #93877

merged 1 commit into from
Feb 19, 2022

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Feb 10, 2022

Previously these were entirely disallowed, except for r11 which was allowed by accident.

cc @hudson-ayers

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 10, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 Feb 10, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jackh726
Copy link
Member

Not my area of expertise

r? rust-lang/compiler

@rust-highfive rust-highfive assigned cjgillot and unassigned jackh726 Feb 12, 2022
@cjgillot
Copy link
Contributor

Not mine either...
r? @joshtriplett
since they reviewed #91643.
Except if @Amanieu has a reviewer in mind.

@Amanieu
Copy link
Member Author

Amanieu commented Feb 17, 2022

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned joshtriplett Feb 17, 2022
nagisa
nagisa reviewed Feb 17, 2022
View reviewed changes
compiler/rustc_target/src/asm/mod.rs
@@ -298,54 +299,87 @@ impl InlineAsmReg {
arch: InlineAsmArch,
target_features: &FxHashSet<Symbol>,
target: &Target,
is_clobber: bool,
Copy link
Member

@nagisa nagisa Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it wouldn't make sense to proactively make this a tri-state enum describing the context (clobber, input, output). I can imagine it being useful when we decide to actually start adding constraint classes that are applicable to input or output only.

And today it would serve as a way to avoid bool proliferation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a followup PR based on this one which is a pain to rebase. I'll make the change there.

@nagisa
Copy link
Member

nagisa commented Feb 17, 2022

LGTM r=me with or without replacement of the bool.

@Amanieu
Copy link
Member Author

Amanieu commented Feb 18, 2022

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit 5ab3483b360dae11f92c9e8fd7a90035934f60e0 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 18, 2022
nbdd0121
nbdd0121 reviewed Feb 18, 2022
View reviewed changes
compiler/rustc_ast_lowering/src/asm.rs Outdated
late,
expr: self.lower_expr_mut(expr),
}
}
InlineAsmOperand::SplitInOut { reg, late, ref in_expr, ref out_expr } => {
hir::InlineAsmOperand::SplitInOut {
reg: lower_reg(reg),
reg: lower_reg(reg, out_expr.is_none()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be false? Would this permit inout("r8") 1 => _?

Copy link
Member

@nagisa nagisa Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's still a clobber just like out("r8") _ is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that's a good point, this is incorrect since we don't want to allow r8 to be used as an input in Thumb1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the name could be clobber_only instead of is_clobber to make it more clear. And we might want an unit test.

@Amanieu
Copy link
Member Author

Amanieu commented Feb 18, 2022

@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 18, 2022
@Amanieu
asm: Allow the use of r8-r14 as clobbers on Thumb1
11250b8 
Previously these were entirely disallowed, except for r11 which was
allowed by accident.
@Amanieu
Copy link
Member Author

Amanieu commented Feb 18, 2022

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit 5ab3483b360dae11f92c9e8fd7a90035934f60e0 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 18, 2022
@Amanieu
Copy link
Member Author

Amanieu commented Feb 18, 2022

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit 11250b8 has been approved by nagisa

@matthiaskrgr matthiaskrgr mentioned this pull request Feb 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
@bors
Auto merge of rust-lang#94134 - matthiaskrgr:rollup-b132kjz, r=matthi…
1882597 
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#89892 (Suggest `impl Trait` return type when incorrectly using a generic return type)
 - rust-lang#91675 (Add MemTagSanitizer Support)
 - rust-lang#92806 (Add more information to `impl Trait` error)
 - rust-lang#93497 (Pass `--test` flag through rustdoc to rustc so `#[test]` functions can be scraped)
 - rust-lang#93814 (mips64-openwrt-linux-musl: correct soft-foat)
 - rust-lang#93847 (kmc-solid: Use the filesystem thread-safety wrapper)
 - rust-lang#93877 (asm: Allow the use of r8-r14 as clobbers on Thumb1)
 - rust-lang#93892 (Only mark projection as ambiguous if GAT substs are constrained)
 - rust-lang#93915 (Implement --check-cfg option (RFC 3013), take 2)
 - rust-lang#93953 (Add the `known-bug` test directive, use it, and do some cleanup)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cb35370 into rust-lang:master Feb 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbdd0121 nbdd0121 nbdd0121 left review comments

@nagisa nagisa nagisa left review comments

@hudson-ayers hudson-ayers hudson-ayers approved these changes

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.60.0
Development

Successfully merging this pull request may close these issues.