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

Fix clobber_abi and disallow SVE-related registers in Arm64EC inline assembly #131332

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

taiki-e
Copy link
Member

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

Currently clobber_abi in Arm64EC inline assembly is implemented using InlineAsmClobberAbi::AArch64NoX18, but broken since it attempts to clobber registers that cannot be used in Arm64EC: https://godbolt.org/z/r3PTrGz5r

error: cannot use register `x13`: x13, x14, x23, x24, x28, v16-v31 cannot be used for Arm64EC
 --> <source>:6:14
  |
6 |     asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
  |              ^^^^^^^^^^^^^^^^

error: cannot use register `x14`: x13, x14, x23, x24, x28, v16-v31 cannot be used for Arm64EC
 --> <source>:6:14
  |
6 |     asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
  |              ^^^^^^^^^^^^^^^^

<omitted the same errors for v16-v31>

Additionally, this disallows SVE-related registers per #131332 (comment).

cc @dpaoliello

r? @Amanieu

@rustbot label O-windows O-AArch64 +A-inline-assembly

@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 Oct 6, 2024
@rustbot rustbot added O-AArch64 Armv8-A or later processors in AArch64 mode O-windows Operating system: Windows labels Oct 6, 2024
Comment on lines 1065 to 1067
p0, p1, p2, p3, p4, p5, p6, p7,
p8, p9, p10, p11, p12, p13, p14, p15,
ffr,
Copy link
Member Author

@taiki-e taiki-e Oct 6, 2024

Choose a reason for hiding this comment

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

Given the reason why v16-v31 are blocked in this target, I suspect that SVE-related registers (z*, p*, ffr) may actually be unavailable in Arm64EC.
Although AFAIK they have not documented anything about it in https://learn.microsoft.com/en-us/windows/arm/arm64ec-abi#register-mapping-and-blocked-registers
(I don't think this question is a blocker for this PR, but I think it could be a blocker for stabilization.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@pmsjt can we get the docs updated to indicate if SVE-related registers (z*, p*, ffr) are available or not on ARM64EC?

Comment on lines 1065 to 1067
p0, p1, p2, p3, p4, p5, p6, p7,
p8, p9, p10, p11, p12, p13, p14, p15,
ffr,
Copy link
Contributor

Choose a reason for hiding this comment

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

@pmsjt can we get the docs updated to indicate if SVE-related registers (z*, p*, ffr) are available or not on ARM64EC?

@pmsjt
Copy link

pmsjt commented Oct 9, 2024

@dpaoliello SVE use in Windows hasn't been documented in either Arm64 classic or Arm64EC. That is coming soon and, when it does, it'll cover both.

To answer your most immediate question: No, SVE should not be used by Arm64EC code. No Arm64EC code should ever access SVE state or ISA regardless of the CPU supporting it. Just as for the presently reserved GP and Neon state in EC [1], SVE will also be reserved and not mapped to any x86-64 state.

[1] https://learn.microsoft.com/en-us/cpp/build/arm64ec-windows-abi-conventions

@taiki-e taiki-e changed the title Fix clobber_abi in Arm64EC inline assembly Fix clobber_abi and disallow preg in Arm64EC inline assembly Oct 9, 2024
@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e marked this pull request as draft October 9, 2024 13:49
@taiki-e taiki-e changed the title Fix clobber_abi and disallow preg in Arm64EC inline assembly Fix clobber_abi and disallow SVE-related registers in Arm64EC inline assembly Oct 9, 2024
@taiki-e taiki-e force-pushed the arm64ec-clobber-abi branch 2 times, most recently from a0f78ea to 3307178 Compare October 9, 2024 14:33
@taiki-e taiki-e marked this pull request as ready for review October 9, 2024 14:34
@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@taiki-e
Copy link
Member Author

taiki-e commented Oct 9, 2024

@pmsjt Thanks for the clarification!

I updated PR to disallow SVE-related registers.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

compiler/rustc_target/src/asm/arm64ec.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/asm/arm64ec.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e force-pushed the arm64ec-clobber-abi branch 2 times, most recently from d380cfd to 5706a48 Compare October 9, 2024 17:40
@rustbot rustbot added the A-inline-assembly Area: Inline assembly (`asm!(…)`) label Oct 13, 2024
@Amanieu
Copy link
Member

Amanieu commented Oct 13, 2024

It's not necessary to disallow the use of the z0-z15 names since internally they are just aliases for v0-v15. All they do is identify the register. What we actually want to disallow is using anything more than 128 bits of the register, which can be done by limiting the types that can be assigned to it. At the moment this is only 64/128-bit NEON types so this is fine, but in the future we will want to reject SVE vector types from #118917 on ARM64EC specifically.

@taiki-e
Copy link
Member Author

taiki-e commented Oct 13, 2024

I see. I removed z0-z15 related changes and added note about SVE vector types.

neon: I8, I16, I32, I64, F16, F32, F64, F128,
VecI8(8), VecI16(4), VecI32(2), VecI64(1), VecF16(4), VecF32(2), VecF64(1),
VecI8(16), VecI16(8), VecI32(4), VecI64(2), VecF16(8), VecF32(4), VecF64(2);
// Note: When adding support for SVE vector types, they must be rejected for Arm64EC.

@taiki-e
Copy link
Member Author

taiki-e commented Oct 13, 2024

I also removed AArch64InlineAsmReg::emit-related change since it is no longer related to this PR. I will open a separate PR for that. UPDATE: opened #131667

@Amanieu
Copy link
Member

Amanieu commented Oct 14, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2024

📌 Commit d858dfe has been approved by Amanieu

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 Oct 14, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 14, 2024
…nieu

Fix clobber_abi and disallow SVE-related registers in Arm64EC inline assembly

Currently `clobber_abi` in Arm64EC inline assembly is implemented using `InlineAsmClobberAbi::AArch64NoX18`, but broken since it attempts to clobber registers that cannot be used in Arm64EC: https://godbolt.org/z/r3PTrGz5r

```
error: cannot use register `x13`: x13, x14, x23, x24, x28, v16-v31 cannot be used for Arm64EC
 --> <source>:6:14
  |
6 |     asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
  |              ^^^^^^^^^^^^^^^^

error: cannot use register `x14`: x13, x14, x23, x24, x28, v16-v31 cannot be used for Arm64EC
 --> <source>:6:14
  |
6 |     asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
  |              ^^^^^^^^^^^^^^^^

<omitted the same errors for v16-v31>
```

Additionally, this disallows SVE-related registers per rust-lang#131332 (comment).

cc `@dpaoliello`

r? `@Amanieu`

`@rustbot` label O-windows O-AArch64 +A-inline-assembly
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#129424 (Stabilize `Pin::as_deref_mut()`)
 - rust-lang#131332 (Fix clobber_abi and disallow SVE-related registers in Arm64EC inline assembly)
 - rust-lang#131384 (Update precondition tests (especially for zero-size access to null))
 - rust-lang#131430 (Special treatment empty tuple when suggest adding a string literal in format macro.)
 - rust-lang#131550 (Make some tweaks to extern block diagnostics)
 - rust-lang#131667 (Fix AArch64InlineAsmReg::emit)
 - rust-lang#131679 (compiletest: Document various parts of compiletest's `lib.rs`)
 - rust-lang#131682 (Tag PRs affecting compiletest with `A-compiletest`)

Failed merges:

 - rust-lang#131496 (Stabilise `const_make_ascii`.)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#129424 (Stabilize `Pin::as_deref_mut()`)
 - rust-lang#131332 (Fix clobber_abi and disallow SVE-related registers in Arm64EC inline assembly)
 - rust-lang#131384 (Update precondition tests (especially for zero-size access to null))
 - rust-lang#131430 (Special treatment empty tuple when suggest adding a string literal in format macro.)
 - rust-lang#131550 (Make some tweaks to extern block diagnostics)
 - rust-lang#131667 (Fix AArch64InlineAsmReg::emit)
 - rust-lang#131679 (compiletest: Document various parts of compiletest's `lib.rs`)
 - rust-lang#131682 (Tag PRs affecting compiletest with `A-compiletest`)

Failed merges:

 - rust-lang#131496 (Stabilise `const_make_ascii`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 43bf4f1 into rust-lang:master Oct 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2024
Rollup merge of rust-lang#131332 - taiki-e:arm64ec-clobber-abi, r=Amanieu

Fix clobber_abi and disallow SVE-related registers in Arm64EC inline assembly

Currently `clobber_abi` in Arm64EC inline assembly is implemented using `InlineAsmClobberAbi::AArch64NoX18`, but broken since it attempts to clobber registers that cannot be used in Arm64EC: https://godbolt.org/z/r3PTrGz5r

```
error: cannot use register `x13`: x13, x14, x23, x24, x28, v16-v31 cannot be used for Arm64EC
 --> <source>:6:14
  |
6 |     asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
  |              ^^^^^^^^^^^^^^^^

error: cannot use register `x14`: x13, x14, x23, x24, x28, v16-v31 cannot be used for Arm64EC
 --> <source>:6:14
  |
6 |     asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
  |              ^^^^^^^^^^^^^^^^

<omitted the same errors for v16-v31>
```

Additionally, this disallows SVE-related registers per rust-lang#131332 (comment).

cc ``@dpaoliello``

r? ``@Amanieu``

``@rustbot`` label O-windows O-AArch64 +A-inline-assembly
@taiki-e taiki-e deleted the arm64ec-clobber-abi branch October 14, 2024 19:29
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Nov 1, 2024
Move remaining inline assembly test files into asm directory

Before:
```
tests/assembly/asm/*
tests/assembly/asm-comments.rs
tests/codegen/asm-target-clobbers.rs
tests/codegen/asm-goto.rs
tests/codegen/asm-maybe-uninit.rs
tests/codegen/asm-msp430-clobbers.rs
tests/codegen/asm-options.rs
tests/codegen/asm-clobbers.rs
tests/codegen/asm-may_unwind.rs
tests/codegen/asm-arm64ec-clobbers.rs
tests/codegen/asm-powerpc-clobbers.rs
tests/codegen/asm-sanitize-llvm.rs
tests/codegen/asm-s390x-clobbers.rs
tests/codegen/asm-clobber_abi.rs
tests/codegen/asm-multiple-options.rs
tests/codegen/global_asm.rs
tests/codegen/global_asm_include.rs
tests/codegen/global_asm_x2.rs
tests/ui/asm/*
```

After:
```
tests/assembly/asm/*
tests/codegen/asm/*
tests/ui/asm/*
```

I moved the remaining standalone test files into the asm directory, and then either removed the "asm-" suffix or for x86-specific registers tests replaced the "asm-" suffix with the "x86-" suffix.

(Then I noticed that there is no test for clobber_abi for already stabilized aarch64, arm, riscv, and loongarch64 asm... I don't believe there is a problem like rust-lang#131332, but I plan to add tests for them later.)

r? `@Amanieu`

`@rustbot` label +A-inline-assembly
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 1, 2024
Move remaining inline assembly test files into asm directory

Before:
```
tests/assembly/asm/*
tests/assembly/asm-comments.rs
tests/codegen/asm-target-clobbers.rs
tests/codegen/asm-goto.rs
tests/codegen/asm-maybe-uninit.rs
tests/codegen/asm-msp430-clobbers.rs
tests/codegen/asm-options.rs
tests/codegen/asm-clobbers.rs
tests/codegen/asm-may_unwind.rs
tests/codegen/asm-arm64ec-clobbers.rs
tests/codegen/asm-powerpc-clobbers.rs
tests/codegen/asm-sanitize-llvm.rs
tests/codegen/asm-s390x-clobbers.rs
tests/codegen/asm-clobber_abi.rs
tests/codegen/asm-multiple-options.rs
tests/codegen/global_asm.rs
tests/codegen/global_asm_include.rs
tests/codegen/global_asm_x2.rs
tests/ui/asm/*
```

After:
```
tests/assembly/asm/*
tests/codegen/asm/*
tests/ui/asm/*
```

I moved the remaining standalone test files into the asm directory, and then either removed the "asm-" suffix or for x86-specific registers tests replaced the "asm-" suffix with the "x86-" suffix.

(Then I noticed that there is no test for clobber_abi for already stabilized aarch64, arm, riscv, and loongarch64 asm... I don't believe there is a problem like rust-lang#131332, but I plan to add tests for them later.)

r? ``@Amanieu``

``@rustbot`` label +A-inline-assembly
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
Rollup merge of rust-lang#132456 - taiki-e:test-asm-dir, r=jieyouxu

Move remaining inline assembly test files into asm directory

Before:
```
tests/assembly/asm/*
tests/assembly/asm-comments.rs
tests/codegen/asm-target-clobbers.rs
tests/codegen/asm-goto.rs
tests/codegen/asm-maybe-uninit.rs
tests/codegen/asm-msp430-clobbers.rs
tests/codegen/asm-options.rs
tests/codegen/asm-clobbers.rs
tests/codegen/asm-may_unwind.rs
tests/codegen/asm-arm64ec-clobbers.rs
tests/codegen/asm-powerpc-clobbers.rs
tests/codegen/asm-sanitize-llvm.rs
tests/codegen/asm-s390x-clobbers.rs
tests/codegen/asm-clobber_abi.rs
tests/codegen/asm-multiple-options.rs
tests/codegen/global_asm.rs
tests/codegen/global_asm_include.rs
tests/codegen/global_asm_x2.rs
tests/ui/asm/*
```

After:
```
tests/assembly/asm/*
tests/codegen/asm/*
tests/ui/asm/*
```

I moved the remaining standalone test files into the asm directory, and then either removed the "asm-" suffix or for x86-specific registers tests replaced the "asm-" suffix with the "x86-" suffix.

(Then I noticed that there is no test for clobber_abi for already stabilized aarch64, arm, riscv, and loongarch64 asm... I don't believe there is a problem like rust-lang#131332, but I plan to add tests for them later.)

r? ``@Amanieu``

``@rustbot`` label +A-inline-assembly
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 10, 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
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
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
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!(…)`) 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-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