powerpc64: improve extern struct ABI #44066
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors: r+ Nice fixes! |
|
📌 Commit fdd889d has been approved by |
alexcrichton
added
the
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 26, 2017
powerpc64: improve extern struct ABI These fixes all have to do with the 64-bit PowerPC ELF ABI for big-endian targets. The ELF v2 ABI for powerpc64le already worked well. - Return after marking return aggregates indirect. Fixes #42757. - Pass one-member float aggregates as direct argument values. - Aggregate arguments less than 64-bit must be written in the least- significant bits of the parameter space. - Larger aggregates are instead padded at the tail. (i.e. filling MSBs, padding the remaining LSBs.) New tests were also added for the single-float aggregate, and a 3-byte aggregate to check that it's filled into LSBs. Overall, at least these formerly-failing tests now pass on powerpc64: - run-make/extern-fn-struct-passing-abi - run-make/extern-fn-with-packed-struct - run-pass/extern-pass-TwoU16s.rs - run-pass/extern-pass-TwoU8s.rs - run-pass/struct-return.rs
|
@bors: r- Apparently this found an existing bug :( |
|
@bors: retry |
|
@bors: retry I've seen a lot of PRs that look like they're still running in github but travis has finished? Like a webhook request to github hasn't happened or something. |
arielb1
added
S-waiting-on-author
|
@bors r- See appveyor failure |
|
For I'll go file that in GCC bugzilla, but what do you suggest we do here in the meantime? |
Perhaps just ignore the test on MinGW? |
cuviper
force-pushed
the
powerpc64-extern-abi
branch
from
fdd889d to
dd8f4eb
Compare
|
OK, I disabled |
|
@bors: r+ |
|
📌 Commit dd8f4eb has been approved by |
|
⌛ Testing commit dd8f4ebf5a784e82da96e49cb08e7de573902618 with merge e933fcfd9b05d0d5372911e68cee3551b6d21721... |
|
💔 Test failed - status-appveyor |
|
Looks like another likely preexising bug? |
|
For i686, Unlike the Win64 case, it's not as clear to me if this is a true ABI problem or if it's just a difference in |
|
Nice investigation! I"m totally ok w/ just ignoring the test for now and waiting to see what upstream says |
|
OK, I filed gcc 82041 and further masked the test. |
|
@bors: r+ |
|
📌 Commit ff9b6f7 has been approved by |
|
⌛ Testing commit ff9b6f7a73a0e9c786b30959c92735db344efbd1 with merge dddd749ea0fe9df7076b8091cfca3fa676199535... |
|
💔 Test failed - status-travis |
|
|
|
I don't have macOS to test on, but using |
|
@cuviper if it helps on a Mac I've got when I compile this code I get this 32-bit assembly and this 64-bit assembly |
|
Thanks! Your 64-bit asm is passing and returning in XMM0, which must be fine since the test did pass on x86_64-apple-darwin (before the job was canceled). Your 32-bit asm returns the value in ST0 like I saw too, with the |
These fixes all have to do with the 64-bit PowerPC ELF ABI for big-endian targets. The ELF v2 ABI for powerpc64le already worked well. - Return after marking return aggregates indirect. Fixes rust-lang#42757. - Pass one-member float aggregates as direct argument values. - Aggregate arguments less than 64-bit must be written in the least- significant bits of the parameter space. - Larger aggregates are instead padded at the tail. (i.e. filling MSBs, padding the remaining LSBs.) New tests were also added for the single-float aggregate, and a 3-byte aggregate to check that it's filled into LSBs. Overall, at least these formerly-failing tests now pass on powerpc64: - run-make/extern-fn-struct-passing-abi - run-make/extern-fn-with-packed-struct - run-pass/extern-pass-TwoU16s.rs - run-pass/extern-pass-TwoU8s.rs - run-pass/struct-return.rs
Following Clang's lead, and anecdotal evidence from the `float_one` part of `run-make/extern-fn-struct-passing-abi`, use a floating point register to return single-float aggregates, except on MSVC targets.
cuviper
force-pushed
the
powerpc64-extern-abi
branch
from
ff9b6f7 to
40b1473
Compare
|
OK, I think that will fix i686. Following Clang's lead, I have |
|
@bors: r+ |
|
📌 Commit 40b1473 has been approved by |
bors
added a commit
that referenced
this pull request
Sep 2, 2017
powerpc64: improve extern struct ABI These fixes all have to do with the 64-bit PowerPC ELF ABI for big-endian targets. The ELF v2 ABI for powerpc64le already worked well. - Return after marking return aggregates indirect. Fixes #42757. - Pass one-member float aggregates as direct argument values. - Aggregate arguments less than 64-bit must be written in the least- significant bits of the parameter space. - Larger aggregates are instead padded at the tail. (i.e. filling MSBs, padding the remaining LSBs.) New tests were also added for the single-float aggregate, and a 3-byte aggregate to check that it's filled into LSBs. Overall, at least these formerly-failing tests now pass on powerpc64: - run-make/extern-fn-struct-passing-abi - run-make/extern-fn-with-packed-struct - run-pass/extern-pass-TwoU16s.rs - run-pass/extern-pass-TwoU8s.rs - run-pass/struct-return.rs
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These fixes all have to do with the 64-bit PowerPC ELF ABI for big-endian
targets. The ELF v2 ABI for powerpc64le already worked well.
significant bits of the parameter space.
(i.e. filling MSBs, padding the remaining LSBs.)
New tests were also added for the single-float aggregate, and a 3-byte
aggregate to check that it's filled into LSBs. Overall, at least these
formerly-failing tests now pass on powerpc64: