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 s390x z13 vector ABI #131586

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 12, 2024

cc #130869

This resolves the following fixmes:

Refs: Section 1.2.3 "Parameter Passing" and section 1.2.5 "Return Values" in ELF Application Binary Interface s390x Supplement, Version 1.6.1 (lzsabi_s390x.pdf in https://github.com/IBM/s390x-abi/releases/tag/v1.6.1)

This PR extends #127731 (merged) 's ABI check to handle cases where vector target feature is disabled.
If we do not do ABI check, we run into the ABI problems as described in #116558 and #130869 (comment), and the problem of the compiler generating strange code (#131586 (comment)).

cc @uweigand

@rustbot label +O-SystemZ +A-ABI

@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 12, 2024
@taiki-e taiki-e mentioned this pull request Oct 12, 2024
10 tasks
@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

r? compiler

@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e force-pushed the s390x-vector-abi branch 3 times, most recently from c14d9b2 to d39e822 Compare October 12, 2024 13:40
@taiki-e

This comment was marked as outdated.

@taiki-e

This comment was marked as outdated.

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. A-ABI Area: Concerning the application binary interface (ABI) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2024
Copy link
Contributor

@uweigand uweigand left a comment

Choose a reason for hiding this comment

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

In addition to the problem mentioned inline, we should also verify that building with soft-float works as expected. This option implicitly disables any use of floating-point or vector registers, and therefore also disables use of the vector facility. I believe the LLVM back-end should handle that logic correctly, but it would be good to have the test check.

*x
}

// FIXME: should check output for z10, but it is very long...
Copy link
Contributor

Choose a reason for hiding this comment

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

It's also incorrect. Without the vector feature, vector types of any length are supposed to be passed via implicit reference, which in the case means a pointer in %r2. The body of the function should simply be:

lg %r2, 0(%r2)

Not sure what exactly the compiler is doing here, but it appears to have scalarized the input and pass it as 8 distinct one-byte arguments ...

Copy link
Member Author

Choose a reason for hiding this comment

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

This case and soft-float case should eventually result in a compilation error once the ABI check is implemented, and I will add UI tests for these cases once the ABI check is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. I just wanted to point out the incorrect code, in case we do end up going back to supporting the no-vector case.

@taiki-e taiki-e force-pushed the s390x-vector-abi branch 2 times, most recently from 0a6b7ad to c1835bd Compare October 14, 2024 11:31
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 14, 2024
@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e force-pushed the s390x-vector-abi branch 2 times, most recently from 546fd91 to 857dbc6 Compare October 14, 2024 15:10
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@taiki-e taiki-e force-pushed the s390x-vector-abi branch 2 times, most recently from a9242d0 to 524c9f6 Compare October 15, 2024 15:54
@bors
Copy link
Contributor

bors commented Oct 16, 2024

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

@bors
Copy link
Contributor

bors commented Oct 25, 2024

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

Comment on lines +7 to +8
// FIXME: +soft-float itself doesn't set -vector
//@[z13_soft_float] compile-flags: --target s390x-unknown-linux-gnu -C target-cpu=z13 -C target-feature=-vector,+soft-float
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a soft-float case per #131586 (review), but the soft-float target feature affects the float ABI, so its use with the -C target-feature flag will likely be deprecated (#116344).

Also, the soft-float target feature is recognized by LLVM but not by rustc, so I get the following warning:

warning: unknown and unstable feature specified for `-Ctarget-feature`: `soft-float`
  |
  = note: it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future

https://godbolt.org/z/x75bPveh5

Copy link
Member Author

Choose a reason for hiding this comment

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

(Btw, it seems other architecture's soft-float targets also explicitly disable SIMD-related features)

features: "-mmx,-sse,-sse2,-sse3,-ssse3,-sse4.1,-sse4.2,-avx,-avx2,+soft-float".into(),

@taiki-e taiki-e marked this pull request as ready for review October 25, 2024 19:32
@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@taiki-e
Copy link
Member Author

taiki-e commented Oct 25, 2024

Rewritten based on the merged #127731.

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 25, 2024
@taiki-e taiki-e requested a review from uweigand October 25, 2024 19:32
@taiki-e
Copy link
Member Author

taiki-e commented Oct 25, 2024

Hmm, #127731 will likely be reverted for now: #132152

@rustbot label +S-blocked

@rustbot rustbot added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 25, 2024
@compiler-errors
Copy link
Member

Sorry for the churn @taiki-e 😭

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-testsuite Area: The testsuite used to check the correctness of rustc O-SystemZ Target: SystemZ processors (s390x) S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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