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 s390x inline assembly #131258

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taiki-e
Copy link
Member

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

This stabilizes inline assembly for s390x (SystemZ).

Corresponding reference PR: rust-lang/reference#1643


From the requirements of stabilization mentioned in #93335

Each architecture needs to be reviewed before stabilization:

  • It must have clobber_abi.

Done in #130630.

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

Done in the PR that added support for clobber_abi.

  • Generally review that the exposed register classes make sense.

The followings can be used as input/output:

  • reg (r[0-10], r[12-14]): General-purpose register

  • reg_addr (r[1-10], r[12-14]): General-purpose register except r0 which is evaluated as zero in an address context

    This class is needed because r0, which may be allocated when using the reg class, cannot be used as a register in certain contexts. This is identical to the a constraint in LLVM and GCC. See Support reg_addr register class in s390x inline assembly #119431 for details.

  • freg (f[0-15]): Floating-point register

The followings are clobber-only:

  • vreg (v[0-31]): Vector register

    Technically vreg should be able to accept #[repr(simd)] types as input/output if the unstable vector target feature added is enabled, but core::arch has no s390x vector type and both #[repr(simd)] and core::simd are unstable. Everything related is unstable, so the fact that this is currently a clobber-only should not be considered a stabilization blocker. (s390x vector facilities support #130869 tracks unstable stuff here)

  • areg (a[2-15]): Access register

All of the above register classes except reg_addr are needed for clobber_abi.

The followings cannot be used as operands for inline asm (see also getReservedRegs and SystemZELFRegisters in LLVM):

  • r11: frame pointer
  • r15: stack pointer
  • a0, a1: Reserved for system use
  • c[0-15] (control register) Reserved by the kernel

Although not listed in the above requirements, preserves_flags is implemented in #111331.


cc @uweigand

r? @Amanieu

@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. O-SystemZ Target: SystemZ processors (s390x) labels Oct 4, 2024
@Amanieu Amanieu added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2024
@Amanieu
Copy link
Member

Amanieu commented Oct 4, 2024

LGTM, but stabilization needs a T-lang FCP.

@taiki-e

This comment was marked as resolved.

@rustbot rustbot added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 9, 2024
@taiki-e
Copy link
Member Author

taiki-e commented Oct 13, 2024

r? lang for FCP

@rustbot rustbot assigned pnkfelix and unassigned Amanieu Oct 13, 2024
@rustbot rustbot added the A-inline-assembly Area: Inline assembly (`asm!(…)`) label Oct 13, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 26, 2024

@taiki-e

The followings are clobber-only:

  • vreg (v[0-31]): Vector register

Technically vreg should be able to accept #[repr(simd)] types as input/output if the unstable vector target feature added is enabled, but core::arch has no s390x vector type and both #[repr(simd)] and core::simd are unstable. Everything related is unstable, so the fact that this is currently a clobber-only should not be considered a stabilization blocker. (#130869 tracks unstable stuff here)

Just to confirm: this is mostly a fairly ordinary "packed SIMD" ISA extension, right? The registers at least are not some exotic ambiguously-sized vector like RISCV's V or AArch64's SVE, but instead have some known fixed size?

@tgross35
Copy link
Contributor

Per #131781 (comment):

@rustbot labels +I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 26, 2024
@taiki-e
Copy link
Member Author

taiki-e commented Oct 26, 2024

@workingjubilee

Just to confirm: this is mostly a fairly ordinary "packed SIMD" ISA extension, right?

Yes. s390x vector facility is fixed-size (packed) SIMD ISA and each vector register is 128 bits long.

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 30, 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 30, 2024
@traviscross
Copy link
Contributor

@rfcbot reviewed

@nikomatsakis
Copy link
Contributor

Glad to see the reference PR as well. I feel good about this. I agree with the general point that it would be good to have a group of people actively maintaining this architecture who can look this over and vet it, but that has more to do with the requirements to be Tier 2 than anything specific to this PR.

@rfcbot required

@tmandry
Copy link
Member

tmandry commented Oct 30, 2024

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 30, 2024
@rfcbot
Copy link

rfcbot commented Oct 30, 2024

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 30, 2024
@RalfJung
Copy link
Member

would be good to have a group of people actively maintaining this architecture who can look this over and vet it, but that has more to do with the requirements to be Tier 2 than anything specific to this PR.

Also see #113739.

@uweigand
Copy link
Contributor

would be good to have a group of people actively maintaining this architecture who can look this over and vet it, but that has more to do with the requirements to be Tier 2 than anything specific to this PR.

Also see #113739.

Not sure what the formal process / requirement for a Tier 2 target maintainer would be - but in any case please feel free to ping me for any s390x related questions.

@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.

@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.

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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. 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-SystemZ Target: SystemZ processors (s390x) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.