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

Stabilize Arm64EC inline assembly #131781

Merged
merged 1 commit into from
Nov 10, 2024
Merged

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Oct 16, 2024

This stabilizes inline assembly for Arm64EC ("Emulation Compatible").

Corresponding reference PR: rust-lang/reference#1653


From the requirements of stabilization mentioned in #93335

Each architecture needs to be reviewed before stabilization:

  • It must have clobber_abi.

Done in #131332.

  • It must be possible to clobber every register that is normally clobbered by a function call.

This is possible from the time of the initial implementation.

  • Generally review that the exposed register classes make sense.

The registers available in this target are a subset of those available in the AArch64 inline assembly which is already stable.

The following registers cannot be used in Arm64EC compared to AArch64:

  • x13, x14, x23, x24, x28 (register class: reg)
  • v[16-31] (register class: vreg)
  • p[0-15], ffr (clobber-only register class preg)

These are disallowed by the ABI (see also abi docs for reg/vreg and #131332 (comment) for preg).

Although not listed in the above requirements, preserves_flags is also implemented and the same as AArch64.


cc @dpaoliello

r? @Amanieu

@rustbot label O-windows O-AArch64 +A-inline-assembly +T-lang -T-compiler +needs-fcp

@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. A-inline-assembly Area: Inline assembly (`asm!(…)`) needs-fcp This change is insta-stable, so needs a completed FCP to proceed. O-AArch64 Armv8-A or later processors in AArch64 mode O-windows Operating system: Windows T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 16, 2024
@rustbot rustbot removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 16, 2024
@dpaoliello
Copy link
Contributor

I'm fine with stabilizing this, but we should include a link to the Microsoft docs: https://learn.microsoft.com/en-us/windows/arm/arm64ec-abi#authoring-arm64ec-in-assembly

At the very least, it should be noted that devs will need to call the appropriate thunks, and I don't believe there's any way to request that new thunks are generated.

@tgross35
Copy link
Contributor

This will probably need lang input like #131258.

@rustbot label +I-lang-easy-decision

@rustbot rustbot added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. label Oct 16, 2024
@Amanieu
Copy link
Member

Amanieu commented Oct 26, 2024

LGTM but needs lang FCP.

r? @joshtriplett

@rustbot rustbot assigned joshtriplett and unassigned Amanieu Oct 26, 2024
@joshtriplett
Copy link
Member

LGTM.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 26, 2024

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 26, 2024
@taiki-e
Copy link
Member Author

taiki-e commented Oct 26, 2024

@dpaoliello

I'm fine with stabilizing this, but we should include a link to the Microsoft docs: https://learn.microsoft.com/en-us/windows/arm/arm64ec-abi#authoring-arm64ec-in-assembly

At the very least, it should be noted that devs will need to call the appropriate thunks, and I don't believe there's any way to request that new thunks are generated.

Thanks for the feedback. I added this to rules for inline assembly in the rust-lang/reference PR.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 26, 2024

At the very least, it should be noted that devs will need to call the appropriate thunks, and I don't believe there's any way to request that new thunks are generated.

@dpaoliello Will devs need to ask for new thunks to be generated, or are there existing thunks they can reach?

@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

Hopefully easy decisions still need a nomination for them to come up on our agenda.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 26, 2024
@dpaoliello
Copy link
Contributor

@dpaoliello Will devs need to ask for new thunks to be generated, or are there existing thunks they can reach?

The thunks have different names and contents depending on the function signature (return and param types and calling conv). So if you are handwriting a function call to an x64 function, a function pointer, or an extern function that you don't know is ARM64EC then you need to call via a thunk with the appropriate name. If LLVM detects that some other code also needs that thunk then it will get generated, if not then you'll get a linker failure for an undefined symbol (the thunk you're calling).

If the dev needs a thunk generated, then they can use cc-rs to build a C file with MSVC that will request the thunk is generated: https://learn.microsoft.com/en-us/windows/arm/arm64ec-abi#generating-entry-and-exit-thunks

So, again, not a blocker but devs must be aware that ARM64EC is ✨special✨ hence why I requested a link to the docs.

@traviscross
Copy link
Contributor

@rfcbot reviewed

1 similar comment
@tmandry
Copy link
Member

tmandry commented Oct 30, 2024

@rfcbot reviewed

@nikomatsakis
Copy link
Contributor

As before I am going to mark this as reviewed but I wish we had clearer lines of ownership

@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Oct 30, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 30, 2024
@bors
Copy link
Contributor

bors commented Nov 2, 2024

☔ The latest upstream changes (presumably #132470) made this pull request unmergeable. Please resolve the merge conflicts.

@taiki-e taiki-e force-pushed the arm64ec-stabilize-asm branch from b4b8a73 to a2763c8 Compare November 2, 2024 04:04
@bors
Copy link
Contributor

bors commented Nov 5, 2024

☔ The latest upstream changes (presumably #131341) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 8, 2024

☔ The latest upstream changes (presumably #132472) made this pull request unmergeable. Please resolve the merge conflicts.

@taiki-e taiki-e force-pushed the arm64ec-stabilize-asm branch from 3357314 to 1d84152 Compare November 8, 2024 01:48
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 9, 2024
@rfcbot
Copy link

rfcbot commented Nov 9, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Nov 9, 2024
@taiki-e taiki-e force-pushed the arm64ec-stabilize-asm branch from 1d84152 to 965a280 Compare November 10, 2024 08:43
@taiki-e
Copy link
Member Author

taiki-e commented Nov 10, 2024

(Rebased to resolve merge conflict with #131258)

@traviscross
Copy link
Contributor

traviscross commented Nov 10, 2024

Given that it looks good to @Amanieu in particular, and has completed FCP...

@bors r=Amanieu,traviscross,joshtriplett

@bors
Copy link
Contributor

bors commented Nov 10, 2024

📌 Commit 965a280 has been approved by Amanieu,traviscross

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 Nov 10, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#131781 (Stabilize Arm64EC inline assembly)
 - rust-lang#132426 (Prefer `pub(super)` in `unreachable_pub` lint suggestion)
 - rust-lang#132866 (Break from review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cdb76c7 into rust-lang:master Nov 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2024
Rollup merge of rust-lang#131781 - taiki-e:arm64ec-stabilize-asm, r=Amanieu,traviscross

Stabilize Arm64EC inline assembly

This stabilizes inline assembly for Arm64EC ("Emulation Compatible").

Corresponding reference PR: rust-lang/reference#1653

---

From the requirements of stabilization mentioned in rust-lang#93335

> Each architecture needs to be reviewed before stabilization:

> - It must have clobber_abi.

Done in rust-lang#131332.

> - It must be possible to clobber every register that is normally clobbered by a function call.

This is possible from the time of the initial implementation.

> - Generally review that the exposed register classes make sense.

The registers available in this target are a subset of those available in the AArch64 inline assembly which is already stable.

The following registers cannot be used in Arm64EC compared to AArch64:

- `x13`, `x14`, `x23`, `x24`, `x28` (register class: `reg`)
- `v[16-31]` (register class: `vreg`)
- `p[0-15]`, `ffr` (clobber-only register class `preg`)

These are disallowed by the ABI (see also [abi docs](https://learn.microsoft.com/en-us/cpp/build/arm64ec-windows-abi-conventions?view=msvc-170#register-mapping) for `reg`/`vreg` and rust-lang#131332 (comment) for `preg`).

Although not listed in the above requirements, preserves_flags is also implemented and the same as AArch64.

---

cc `@dpaoliello`

r? `@Amanieu`

`@rustbot` label O-windows O-AArch64 +A-inline-assembly +T-lang -T-compiler +needs-fcp
@taiki-e taiki-e deleted the arm64ec-stabilize-asm branch November 11, 2024 01:36
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…manieu,traviscross

Stabilize Arm64EC inline assembly

This stabilizes inline assembly for Arm64EC ("Emulation Compatible").

Corresponding reference PR: rust-lang/reference#1653

---

From the requirements of stabilization mentioned in rust-lang#93335

> Each architecture needs to be reviewed before stabilization:

> - It must have clobber_abi.

Done in rust-lang#131332.

> - It must be possible to clobber every register that is normally clobbered by a function call.

This is possible from the time of the initial implementation.

> - Generally review that the exposed register classes make sense.

The registers available in this target are a subset of those available in the AArch64 inline assembly which is already stable.

The following registers cannot be used in Arm64EC compared to AArch64:

- `x13`, `x14`, `x23`, `x24`, `x28` (register class: `reg`)
- `v[16-31]` (register class: `vreg`)
- `p[0-15]`, `ffr` (clobber-only register class `preg`)

These are disallowed by the ABI (see also [abi docs](https://learn.microsoft.com/en-us/cpp/build/arm64ec-windows-abi-conventions?view=msvc-170#register-mapping) for `reg`/`vreg` and rust-lang#131332 (comment) for `preg`).

Although not listed in the above requirements, preserves_flags is also implemented and the same as AArch64.

---

cc `@dpaoliello`

r? `@Amanieu`

`@rustbot` label O-windows O-AArch64 +A-inline-assembly +T-lang -T-compiler +needs-fcp
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#131781 (Stabilize Arm64EC inline assembly)
 - rust-lang#132426 (Prefer `pub(super)` in `unreachable_pub` lint suggestion)
 - rust-lang#132866 (Break from review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
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!(…)`) disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. O-AArch64 Armv8-A or later processors in AArch64 mode O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.