Refactor `powerpc64` call ABI handling by beetrees · Pull Request #128643 · rust-lang/rust

beetrees · 2024-08-04T14:21:04Z

As the specification for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the classify_arg and classify_ret functions to reduce code duplication. The only functional change is to fix #128579: the classify_ret function was incorrectly handling aggregates where bits > 64 && bits < 128. I've used the aggregate handling implementation from classify_arg which doesn't have this issue.

@awilfox could you test this on powerpc64-unknown-linux-musl? I'm only able to cross-test on powerpc64-unknown-linux-gnu and powerpc64le-unknown-linux-gnu locally at the moment, and as a tier 3 target powerpc64-unknown-linux-musl has zero CI coverage.

Fixes: #128579

rustbot · 2024-08-04T14:21:12Z

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

jieyouxu · 2024-08-04T14:25:46Z

r? @bjorn3 (feel free to reroll, or, I can open a T-compiler thread asking for a suitable reviewer)

jieyouxu · 2024-08-04T14:38:31Z

cc @ecnelises and @bzEq since you were listed as powerpc64-ibm-aix target maintainers, and this affects your target AFAICT.

tgross35 · 2024-08-04T18:17:15Z

Is a codegen regression test possible here?

workingjubilee · 2024-08-04T19:02:41Z

An assembly test would be nice.

awilfox · 2024-08-05T01:37:44Z

Thank you! This looks right to me. I'll do a test run on my PPC64 musl workstation soon (it is currently doing some LLVM work) and will let you know the result.

bzEq · 2024-08-09T07:23:37Z

cc @ecnelises and @bzEq since you were listed as powerpc64-ibm-aix target maintainers, and this affects your target AFAICT.

Thanks for heads up. However, AIX has its own ABI and is not using ELFv1/v2. Anyway, I'll test this PR on AIX.

bzEq · 2024-08-09T07:35:10Z

Further, I don't think AIX ABI is a blocker for this PR. If it breaks AIX in our buildbot, we will upstream AIX change.

beetrees · 2024-08-09T20:36:12Z

I've added an assembly test for the PowerPC64 struct ABI.

awilfox · 2024-08-09T23:41:19Z

I've just queued the latest version of this patch (dd2fc07) on my powerpc64-musl build system, and will let you know the result in about two hours when it finishes.

awilfox · 2024-08-10T01:22:49Z

All tests pass here on powerpc64-musl (BE)! Including the new tests that the CI bot seems to believe are failing:

test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv1-be ... ok
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-be ... ok
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-le ... ok

This system is using LLVM 18.1.8, so perhaps it is an LLVM 17 issue.

beetrees · 2024-08-10T05:30:23Z

I've fixed the test so that it works on LLVM 17.

tests/assembly/powerpc64-struct-abi.rs

compiler/rustc_target/src/abi/call/powerpc64.rs

bjorn3 · 2024-08-10T10:35:18Z

By the way did you try abi-cafe with these changes? It is pretty good at finding abi divergences between compilers.

beetrees · 2024-08-10T18:36:08Z

Haven't tried it yet; I'll give it a go on powerpc64-unknown-linux-gnu and powerpc64le-unknown-linux-gnu. @awilfox think you could give abi-cafe a run on powerpc64-unknown-linux-musl? It'll help check if there are any remaining ABI incompatibilities between rustc and gcc/clang.

bjorn3 · 2024-08-10T19:03:10Z

r=me if abi-cafe passes.

beetrees · 2024-08-11T01:03:18Z

I've ran abi-cafe on powerpc64-unknown-linux-gnu and powerpc64le-unknown-linux-gnu. The only failures were pre-existing failures of some u128 and i128 tests. The root cause of this appears to be that Rust thinks that u128/i128 have an alignment of 8 whereas the ABI specification and gcc/clang think u128/i128 have an alignment of 16. This is essentially another instance of #54341 but on a different platform (cc rust-lang/lang-team#255). Like in that issue, the proper solution here is to fix the LLVM data layout.

awilfox · 2024-08-11T01:51:52Z

I see the same on powerpc64-musl here; i128/u128 failures but otherwise abi-cafe passes with this patch applied, both with 1.80.0 and nightly.

bjorn3 · 2024-08-11T06:23:43Z

Thanls for testing. The u128/i128 issue can be fixed later.

@bors r+

bors · 2024-08-11T06:23:45Z

📌 Commit 6308d13 has been approved by bjorn3

It is now in the queue for this repository.

Refactor `powerpc64` call ABI handling As the [specification](https://openpowerfoundation.org/specifications/64bitelfabi/) for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the `classify_arg` and `classify_ret` functions to reduce code duplication. The only functional change is to fix rust-lang#128579: the `classify_ret` function was incorrectly handling aggregates where `bits > 64 && bits < 128`. I've used the aggregate handling implementation from `classify_arg` which doesn't have this issue. `@awilfox` could you test this on `powerpc64-unknown-linux-musl`? I'm only able to cross-test on `powerpc64-unknown-linux-gnu` and `powerpc64le-unknown-linux-gnu` locally at the moment, and as a tier 3 target `powerpc64-unknown-linux-musl` has zero CI coverage. Fixes: rust-lang#128579

Refactor `powerpc64` call ABI handling As the [specification](https://openpowerfoundation.org/specifications/64bitelfabi/) for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the `classify_arg` and `classify_ret` functions to reduce code duplication. The only functional change is to fix rust-lang#128579: the `classify_ret` function was incorrectly handling aggregates where `bits > 64 && bits < 128`. I've used the aggregate handling implementation from `classify_arg` which doesn't have this issue. ``@awilfox`` could you test this on `powerpc64-unknown-linux-musl`? I'm only able to cross-test on `powerpc64-unknown-linux-gnu` and `powerpc64le-unknown-linux-gnu` locally at the moment, and as a tier 3 target `powerpc64-unknown-linux-musl` has zero CI coverage. Fixes: rust-lang#128579

Rollup of 5 pull requests Successful merges: - rust-lang#128643 (Refactor `powerpc64` call ABI handling) - rust-lang#128873 (Add windows-targets crate to std's sysroot) - rust-lang#128916 (Tidy up `dump-ice-to-disk` and make assertion failures dump ICE messages) - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago) - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2024-08-11T12:56:13Z

Failed in #128968 (comment).
@bors r-

test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-le ... FAILED
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv2-be ... FAILED
test [assembly] tests/assembly/powerpc64-struct-abi.rs#elfv1-be ... FAILED

beetrees · 2024-08-11T13:13:50Z

I've fixed the test.

bjorn3 · 2024-08-11T13:16:47Z

@bors r+

bors · 2024-08-11T13:16:49Z

📌 Commit 715728f has been approved by bjorn3

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128643 (Refactor `powerpc64` call ABI handling) - rust-lang#128655 (std: refactor UNIX random data generation) - rust-lang#128745 (Remove unused lifetime parameter from spawn_unchecked) - rust-lang#128841 (bootstrap: don't use rustflags for `--rustc-args`) - rust-lang#128983 (Slightly refactor `TargetSelection` in bootstrap) - rust-lang#129026 (CFI: Move CFI ui tests to cfi directory) - rust-lang#129040 (Fix blessing of rmake tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128643 - beetrees:ppc64-abi-fix, r=bjorn3 Refactor `powerpc64` call ABI handling As the [specification](https://openpowerfoundation.org/specifications/64bitelfabi/) for the ELFv2 ABI states that returned aggregates are returned like arguments as long as they are at most two doublewords, I've merged the `classify_arg` and `classify_ret` functions to reduce code duplication. The only functional change is to fix rust-lang#128579: the `classify_ret` function was incorrectly handling aggregates where `bits > 64 && bits < 128`. I've used the aggregate handling implementation from `classify_arg` which doesn't have this issue. `@awilfox` could you test this on `powerpc64-unknown-linux-musl`? I'm only able to cross-test on `powerpc64-unknown-linux-gnu` and `powerpc64le-unknown-linux-gnu` locally at the moment, and as a tier 3 target `powerpc64-unknown-linux-musl` has zero CI coverage. Fixes: rust-lang#128579

rustbot assigned jieyouxu Aug 4, 2024

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 Aug 4, 2024

rustbot assigned bjorn3 and unassigned jieyouxu Aug 4, 2024

jieyouxu added the O-PowerPC Target: PowerPC processors label Aug 4, 2024

jieyouxu added the A-ABI Area: Concerning the application binary interface (ABI) label Aug 4, 2024

beetrees force-pushed the ppc64-abi-fix branch from d6c1ed0 to 41e352d Compare August 9, 2024 20:35

This comment has been minimized.

Sign in to view

beetrees force-pushed the ppc64-abi-fix branch from 41e352d to dd2fc07 Compare August 9, 2024 23:28

This comment has been minimized.

Sign in to view

beetrees force-pushed the ppc64-abi-fix branch 2 times, most recently from 1c55e73 to 6308d13 Compare August 10, 2024 02:02

bjorn3 reviewed Aug 10, 2024

View reviewed changes

tests/assembly/powerpc64-struct-abi.rs Show resolved Hide resolved

bjorn3 reviewed Aug 10, 2024

View reviewed changes

compiler/rustc_target/src/abi/call/powerpc64.rs Show resolved Hide resolved

beetrees mentioned this pull request Aug 11, 2024

Several platforms still have incorrectly aligned u128/i128 #128950

Closed

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 Aug 11, 2024

jieyouxu mentioned this pull request Aug 11, 2024

Rollup of 5 pull requests #128966

Closed

jieyouxu mentioned this pull request Aug 11, 2024

Rollup of 5 pull requests #128968

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 11, 2024

Refactor powerpc64 call ABI handling

715728f

beetrees force-pushed the ppc64-abi-fix branch from 6308d13 to 715728f Compare August 11, 2024 13:13

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2024

matthiaskrgr mentioned this pull request Aug 13, 2024

Rollup of 7 pull requests #129046

Merged

bors merged commit 00d040e into rust-lang:master Aug 13, 2024

rustbot added this to the 1.82.0 milestone Aug 13, 2024

beetrees deleted the ppc64-abi-fix branch August 16, 2024 01:13

bzEq mentioned this pull request Oct 5, 2024

ABI: Pass aggregates by value on AIX #131208

Merged

Uh oh!

Conversation

beetrees commented Aug 4, 2024

Uh oh!

rustbot commented Aug 4, 2024

Uh oh!

jieyouxu commented Aug 4, 2024

Uh oh!

jieyouxu commented Aug 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 commented Aug 4, 2024

Uh oh!

workingjubilee commented Aug 4, 2024

Uh oh!

awilfox commented Aug 5, 2024

Uh oh!

bzEq commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bzEq commented Aug 9, 2024

Uh oh!

beetrees commented Aug 9, 2024

Uh oh!

This comment has been minimized.

awilfox commented Aug 9, 2024

Uh oh!

This comment has been minimized.

awilfox commented Aug 10, 2024

Uh oh!

beetrees commented Aug 10, 2024

Uh oh!

Uh oh!

Uh oh!

bjorn3 commented Aug 10, 2024

Uh oh!

beetrees commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 commented Aug 10, 2024

Uh oh!

beetrees commented Aug 11, 2024

Uh oh!

awilfox commented Aug 11, 2024

Uh oh!

bjorn3 commented Aug 11, 2024

Uh oh!

bors commented Aug 11, 2024

Uh oh!

jieyouxu commented Aug 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beetrees commented Aug 11, 2024

Uh oh!

bjorn3 commented Aug 11, 2024

Uh oh!

bors commented Aug 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jieyouxu commented Aug 4, 2024 •

edited

Loading

bzEq commented Aug 9, 2024 •

edited

Loading

beetrees commented Aug 10, 2024 •

edited

Loading

jieyouxu commented Aug 11, 2024 •

edited

Loading