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

Support input/output in vector registers of s390x inline assembly (under asm_experimental_reg feature) #131664

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

taiki-e
Copy link
Member

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

This extends currently clobber-only vector registers (vreg) support to allow passing #[repr(simd)] types, floats (f32/f64/f128), and integers (i32/i64/i128) as input/output.

This is unstable and gated under new #![feature(asm_experimental_reg)] (tracking issue: #133416). If the feature is not enabled, only clober is supported as before.

Architecture Register class Target feature Allowed types
s390x vreg vector i32, f32, i64, f64, i128, f128, i8x16, i16x8, i32x4, i64x2, f32x4, f64x2

This matches the list of types that are supported by the vector registers in LLVM:
https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/SystemZ/SystemZRegisterInfo.td#L301-L313

In addition to core::simd types and floats listed above, custom #[repr(simd)] types of the same size and type are also allowed. All allowed types other than i32/f32/i64/f64/i128, and relevant target features are currently unstable.

Currently there is no SIMD type for s390x in core::arch, but this is tracked in #130869.

cc #130869 about vector facility support in s390x
cc #125398 & #116909 about f128 support in asm

@rustbot label +O-SystemZ +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. A-inline-assembly Area: Inline assembly (`asm!(…)`) O-SystemZ Target: SystemZ processors (s390x) S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 13, 2024
@taiki-e taiki-e mentioned this pull request Oct 13, 2024
11 tasks
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2024
@bors
Copy link
Contributor

bors commented Oct 14, 2024

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

@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from 107fef5 to 57419c1 Compare October 25, 2024 19:42
@bors
Copy link
Contributor

bors commented Oct 26, 2024

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

@bors
Copy link
Contributor

bors commented Nov 14, 2024

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

@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from 57419c1 to a9476db Compare November 21, 2024 17:34
@taiki-e taiki-e changed the title Support #[repr(simd)] types in input/output of s390x inline assembly Support #[repr(simd)] types and floats in input/output of s390x inline assembly Nov 21, 2024
@taiki-e taiki-e marked this pull request as ready for review November 21, 2024 17:35
@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from a9476db to d0a124a Compare November 21, 2024 17:52
@taiki-e taiki-e changed the title Support #[repr(simd)] types and floats in input/output of s390x inline assembly Support #[repr(simd)] types in input/output of s390x inline assembly Nov 21, 2024
@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch 2 times, most recently from d0a124a to 51506f9 Compare November 21, 2024 18:40
@taiki-e taiki-e changed the title Support #[repr(simd)] types in input/output of s390x inline assembly Support #[repr(simd)] types and floats in input/output of s390x inline assembly Nov 21, 2024
@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from 51506f9 to b6b9f79 Compare November 21, 2024 19:17
@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from b6b9f79 to 2c8f6de Compare November 21, 2024 19:18
@taiki-e taiki-e changed the title Support #[repr(simd)] types and floats in input/output of s390x inline assembly Support input/output in vector registers of s390x inline assembly Nov 21, 2024
@taiki-e
Copy link
Member Author

taiki-e commented Nov 21, 2024

Vector ABI support (#131586) has been merged, and this is now ready for review.

cc @uweigand
r? @Amanieu

@rustbot label -S-blocked +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 21, 2024
@Amanieu
Copy link
Member

Amanieu commented Nov 21, 2024

Is there a way to feature-gate this functionality? Since s390x asm support is stable, every change here needs to go through an FCP since it will instantly be stable.

@taiki-e
Copy link
Member Author

taiki-e commented Nov 22, 2024

Is there a way to feature-gate this functionality?

Yes, I implemented it in another branch (taiki-e/rust@s390x-asm-vreg-inout...s390x-asm-vreg-inout-unstable):

error[E0658]: this register class can only be used as a clobber in stable
  --> $DIR/bad-reg.rs:74:9
   |
LL |         asm!("", in("v0") v); // requires vector & asm_experimental_arch
   |         ^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #93335 <https://github.com/rust-lang/rust/issues/93335> for more information
   = help: add `#![feature(asm_experimental_arch)]` to the crate attributes to enable
   = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

Here I'm using the existing asm_experimental_arch as a gate, but it might make more sense to add a dedicated gate such as asm_experimental_reg.

@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@taiki-e taiki-e changed the title Support input/output in vector registers of s390x inline assembly Support input/output in vector registers of s390x inline assembly (under asm_experimental_reg feature) Nov 24, 2024
@taiki-e
Copy link
Member Author

taiki-e commented Nov 24, 2024

Added new asm_experimental_reg feature (tracking issue: #133416) and moved s390x non-clobber-only vector register support under that feature.

error[E0658]: this register class can only be used as a clobber in stable
  --> $DIR/bad-reg.rs:74:9
   |
LL |         asm!("", in("v0") v); // requires vector & asm_experimental_reg
   |         ^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #133416 <https://github.com/rust-lang/rust/issues/133416> for more information
   = help: add `#![feature(asm_experimental_reg)]` to the crate attributes to enable
   = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from d2357fb to 9b0b4c8 Compare November 24, 2024 11:25
let reg_size =
reg_class.supported_types(self.arch).iter().map(|(ty, _)| ty.size()).max().unwrap();
let reg_size = reg_class
.supported_types(self.arch, self.allow_experimental_reg)
Copy link
Member

Choose a reason for hiding this comment

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

At the codegen stage we can just use true here instead of allowed_experimental_reg. All the stability checking will have been done in priot stages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed rustc_codegen_* to just use true.

I also left a document about the cases in which it is fine to always pass true.

/// At the codegen stage, it is fine to always pass true for `allow_experimental_reg`,
/// since all the stability checking will have been done in prior stages.
pub fn supported_types(

let reg_class = reg.reg_class();
let supported_tys = reg_class.supported_types(asm_arch);
let supported_tys = reg_class.supported_types(asm_arch, allow_experimental_reg);
let Some((_, feature)) = supported_tys.iter().find(|&&(t, _)| t == asm_ty) else {
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to handle the case where a given type is only supported with allow_experimental_reg and report an appropriate error message. This would be used in the future for SVE support for example since it adds more types to an existing register class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

error[E0658]: type `i32` cannot be used with this register class in stable
--> $DIR/feature-gate-asm_experimental_reg.rs:20:23
|
LL | asm!("", in("v0") 0);
| ^
|
= note: see issue #133416 <https://github.com/rust-lang/rust/issues/133416> for more information
= help: add `#![feature(asm_experimental_reg)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from 9b0b4c8 to 7bd6de8 Compare November 24, 2024 12:28
@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e force-pushed the s390x-asm-vreg-inout branch from 7bd6de8 to c024d8c Compare November 24, 2024 12:42
@Amanieu
Copy link
Member

Amanieu commented Nov 25, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2024

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

Rollup of 8 pull requests

Successful merges:

 - rust-lang#131523 (Fix asm goto with outputs and move it to a separate feature gate)
 - rust-lang#131664 (Support input/output in vector registers of s390x inline assembly (under asm_experimental_reg feature))
 - rust-lang#132432 (Add a test to verify that libstd doesn't use protected symbols)
 - rust-lang#132502 (Document possibility to set core features in example config.toml)
 - rust-lang#132529 (ci(triagebot): add more top-level files to A-meta)
 - rust-lang#132533 (Add BorrowedBuf::into_filled{,_mut} methods to allow returning buffer with original lifetime)
 - rust-lang#132803 (Fix broken url)
 - rust-lang#132982 (alloc: fix `Allocator` method names in `alloc` free function docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3f86edd into rust-lang:master Nov 25, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 25, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2024
Rollup merge of rust-lang#131664 - taiki-e:s390x-asm-vreg-inout, r=Amanieu

Support input/output in vector registers of s390x inline assembly (under asm_experimental_reg feature)

This extends currently clobber-only vector registers (`vreg`) support to allow passing `#[repr(simd)]` types, floats (f32/f64/f128), and integers (i32/i64/i128) as input/output.

This is unstable and gated under new `#![feature(asm_experimental_reg)]` (tracking issue: rust-lang#133416). If the feature is not enabled, only clober is supported as before.

| Architecture | Register class | Target feature | Allowed types |
| ------------ | -------------- | -------------- | -------------- |
| s390x | `vreg` | `vector` | `i32`, `f32`, `i64`, `f64`, `i128`, `f128`, `i8x16`, `i16x8`, `i32x4`, `i64x2`, `f32x4`, `f64x2` |

This matches the list of types that are supported by the vector registers in LLVM:
https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/SystemZ/SystemZRegisterInfo.td#L301-L313

In addition to `core::simd` types and floats listed above, custom `#[repr(simd)]` types of the same size and type are also allowed. All allowed types other than i32/f32/i64/f64/i128, and relevant target features are currently unstable.

Currently there is no SIMD type for s390x in `core::arch`, but this is tracked in rust-lang#130869.

cc rust-lang#130869 about vector facility support in s390x
cc rust-lang#125398 & rust-lang#116909 about f128 support in asm

`@rustbot` label +O-SystemZ +A-inline-assembly
@taiki-e taiki-e deleted the s390x-asm-vreg-inout branch November 25, 2024 11:10
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Dec 6, 2024
…anieu

Support input/output in vector registers of s390x inline assembly (under asm_experimental_reg feature)

This extends currently clobber-only vector registers (`vreg`) support to allow passing `#[repr(simd)]` types, floats (f32/f64/f128), and integers (i32/i64/i128) as input/output.

This is unstable and gated under new `#![feature(asm_experimental_reg)]` (tracking issue: rust-lang#133416). If the feature is not enabled, only clober is supported as before.

| Architecture | Register class | Target feature | Allowed types |
| ------------ | -------------- | -------------- | -------------- |
| s390x | `vreg` | `vector` | `i32`, `f32`, `i64`, `f64`, `i128`, `f128`, `i8x16`, `i16x8`, `i32x4`, `i64x2`, `f32x4`, `f64x2` |

This matches the list of types that are supported by the vector registers in LLVM:
https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/SystemZ/SystemZRegisterInfo.td#L301-L313

In addition to `core::simd` types and floats listed above, custom `#[repr(simd)]` types of the same size and type are also allowed. All allowed types other than i32/f32/i64/f64/i128, and relevant target features are currently unstable.

Currently there is no SIMD type for s390x in `core::arch`, but this is tracked in rust-lang#130869.

cc rust-lang#130869 about vector facility support in s390x
cc rust-lang#125398 & rust-lang#116909 about f128 support in asm

`@rustbot` label +O-SystemZ +A-inline-assembly
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-SystemZ Target: SystemZ processors (s390x) 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.

5 participants