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

Add -Zfixed-x18 #124655

Merged
merged 6 commits into from
May 29, 2024
Merged

Add -Zfixed-x18 #124655

merged 6 commits into from
May 29, 2024

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented May 3, 2024 •
edited
Loading

This PR is a follow-up to #124323 that proposes a different implementation. Please read the description of that PR for motivation.

See the equivalent flag in the clang docs.

MCP: rust-lang/compiler-team#748
Fixes #121970
r? rust-lang/compiler

@Darksonn
Add -Zfixed-x18
40f0172 
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 3, 2024
@Darksonn Darksonn mentioned this pull request May 3, 2024
Closed
@ojeda ojeda mentioned this pull request May 3, 2024
Open
52 tasks
@estebank
Copy link
Contributor

estebank commented May 7, 2024

r? @RalfJung

It seems like the right person to review this? The changes look reasonable to me.

@rustbot rustbot assigned RalfJung and unassigned estebank May 7, 2024
@RalfJung
Copy link
Member

RalfJung commented May 8, 2024

Sorry, I can't do reviews outside the interpreter.
r? compiler

@rustbot rustbot assigned oli-obk and unassigned RalfJung May 8, 2024
@Darksonn
Copy link
Contributor Author

Darksonn commented May 9, 2024

@apiraino is an MCP needed?

@apiraino
Copy link
Contributor

apiraino commented May 14, 2024 •
edited
Loading

@apiraino is an MCP needed?

Posting here the answer from compiler team (see Zulip): it's to review this without MCP since it's an unstable flag

@rust-log-analyzer

This comment has been minimized.

Sign in to view
Darksonn
Darksonn commented May 15, 2024
View reviewed changes
compiler/rustc_codegen_llvm/src/llvm_util.rs
// -Zfixed-x18
if sess.opts.unstable_opts.fixed_x18 {
if sess.target.arch != "aarch64" {
sess.dcx().emit_fatal(FixedX18InvalidArch { arch: &sess.target.arch });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used emit_fatal here because with emit_err, the error is printed twice.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Darksonn Darksonn mentioned this pull request May 15, 2024
Closed
3 tasks
@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2024

r? @lqd

@rustbot rustbot assigned lqd and unassigned oli-obk May 22, 2024
@lqd
Copy link
Member

lqd commented May 22, 2024

@oli-obk sure, I can review when the MCP completes if you'd like (I think @estebank already approved)

@Darksonn
Copy link
Contributor Author

The MCP has completed.

@lqd
Copy link
Member

lqd commented May 29, 2024

We probably don't need all the blessed expectations compared to e.g.

//@ error-pattern: the `-Zfixed-x18` flag is not supported
//@ dont-check-compiler-stderr

but it's fine.

bsmith mentioned

cc-rs would need to be updated to forward this option

Does this need to be done soon after merging this PR, or when the full design is finalized for stabilization?

On zulip, you, ralf and bjorn were still discussing details, but it's not clear to me whether these mention possible future work, or an approach you three would rather take in this PR? If it's the former, this looks good enough to land unstably to me, and we can iterate from there if need be.

@Darksonn
Copy link
Contributor Author

cc @NobodyXu on the cc-rs question

On zulip, you, ralf and bjorn were still discussing details, but it's not clear to me whether these mention possible future work, or an approach you three would rather take in this PR?

I would rather land this PR now as unstable and change it later.

@lqd
Copy link
Member

lqd commented May 29, 2024

That seems good to me as well if RfL is fine with the possible churn, as I'd expect you to be the only consumers for this flag for a while.

Thanks!
@bors r=lqd,estebank

@bors
Copy link
Contributor

bors commented May 29, 2024

📌 Commit 4aafecb has been approved by lqd,estebank

It is now in the queue for this repository.

@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 29, 2024
@ojeda
Copy link
Contributor

ojeda commented May 29, 2024 •
edited
Loading

That seems good to me as well if RfL is fine with the possible churn, as I'd expect you to be the only consumers for this flag for a while.

It should be fine, and this way we can start getting some usage/testing out of it, including in the potential/upcoming Rust for Linux CI job here (where churn should not matter, in the sense that we can change the job at the same time in the same PR to adapt). If we really expect a lot of churn, we could consider avoiding to put it in upstream Linux just yet, but I think it would still be fine.

Thanks Rémy!

@matthiaskrgr matthiaskrgr mentioned this pull request May 29, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2024
@bors
Auto merge of rust-lang#125732 - matthiaskrgr:rollup-bozbtk3, r=matth…
debd22d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124655 (Add `-Zfixed-x18`)
 - rust-lang#125693 (Format all source files in `tests/coverage/`)
 - rust-lang#125700 (coverage: Avoid overflow when the MC/DC condition limit is exceeded)
 - rust-lang#125705 (Reintroduce name resolution check for trying to access locals from an inline const)
 - rust-lang#125708 (tier 3 target policy: clarify the point about producing assembly)
 - rust-lang#125715 (remove unneeded extern crate in rmake test)
 - rust-lang#125719 (Extract coverage-specific code out of `compiletest::runtest`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d0311c1 into rust-lang:master May 29, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2024
@rust-timer
Unrolled build for rust-lang#124655
d0297fa 
Rollup merge of rust-lang#124655 - Darksonn:fixed-x18, r=lqd,estebank

Add `-Zfixed-x18`

This PR is a follow-up to rust-lang#124323 that proposes a different implementation. Please read the description of that PR for motivation.

See the equivalent flag in [the clang docs](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffixed-x18).

MCP: rust-lang/compiler-team#748
Fixes rust-lang#121970
r? rust-lang/compiler
@fmease
Copy link
Member

fmease commented May 29, 2024

bors sleepy @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 May 29, 2024
@ojeda ojeda mentioned this pull request May 30, 2024
Open
90 tasks
@NobodyXu
Copy link
Contributor

cc @NobodyXu on the cc-rs question

On zulip, you, ralf and bjorn were still discussing details, but it's not clear to me whether these mention possible future work, or an approach you three would rather take in this PR?

I would rather land this PR now as unstable and change it later.

Do you mean:

That way, cc-rs can translate -Ctarget-feature=-x18-available into -ffixed-x18 as well, instead of emitting -ffixed-x18 when you do nothing.

it sounds alright to me

@Darksonn Darksonn deleted the fixed-x18 branch May 30, 2024 07:17
@Darksonn
Copy link
Contributor Author

@NobodyXu What landed yesterday is a -Zfixed-x18 flag that needs to be converted to -ffixed-x18. Potentially we change the flag to an x18-available feature in the future. Does cc-rs need to support -Zfixed-x18 now?

@NobodyXu
Copy link
Contributor

What landed yesterday is a -Zfixed-x18 flag that needs to be converted to -ffixed-x18. Potentially we change the flag to an x18-available feature in the future. Does cc-rs need to support -Zfixed-x18 now?

Thanks for explanation, currently nobody requested for that feature, but it might be a feature good to have just in case someone needs it.

@taiki-e taiki-e mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqd lqd lqd left review comments

@estebank estebank estebank approved these changes

Assignees

@lqd lqd

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.

Compiler does not recognize the reserve-x18 target feature