Fix passing/returning structs with the 64-bit SPARC ABI #142680
Conversation
|
|
|
r? @bjorn3 |
|
Note that there are currently two places that Clang and GCC disagree AFAIK:
|
|
Thank you for doing this! My knowledge in this area is rather limited. I was able to build latest Rust sources with your fix on top on Solaris 11.4 SPARC. Then I used this version to build Firefox on Solaris. There was no problem. Firefox also worked as expected. I also tested some problematic cases which I was handling few years ago (https://github.com/psumbera/rust-sparc64-abi-tests). All seemed to be ok. |
|
I don't have time to properly review this right now. r? compiler |
|
☔ The latest upstream changes (presumably #143521) made this pull request unmergeable. Please resolve the merge conflicts. |
4eb3ce5 to
9e4a7bc
Compare
|
Rebased |
|
☔ The latest upstream changes (presumably #144249) made this pull request unmergeable. Please resolve the merge conflicts. |
9e4a7bc to
b7b3ed7
Compare
|
Hi @workingjubilee, are you still interested in reviewing this? If not, we can reroll for a different reviewer. Thanks! |
|
Looks like there is a Clang fix in llvm/llvm-project#155829 |
There was a problem hiding this comment.
Do these need fn(T) -> T signatures? I've found abi tests a bit easier to follow if split to fn(T, &mutT) and fn(&T) -> T
| @@ -66,3 +82,58 @@ pub unsafe extern "C" fn caller() { | |||
| opaque_callee(Franta { a: 1.0, b: 2.0, c: 3.0, d: 4.0 }, 3); | |||
| tail_call_avoidance_fn(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Mind adding comments about what these are showing?
| return; | ||
| continue; |
There was a problem hiding this comment.
Huh, was this just ignoring all args after a ZST? This would probably be a good test to add.
| // FIXME(llvm/llvm-project#97981): f16 doesn't have a proper ABI in LLVM on | ||
| // sparc64 yet. Once it does, double-check if it needs to be passed in a | ||
| // floating-point register here. |
There was a problem hiding this comment.
Maybe worth just updating to what Clang has now? (Since the sparc part of that issue was addressed). I don't think they would update again unless GCC does something different
| // CHECK-NEXT: fmovs %f2, %f0 | ||
| // CHECK-NEXT: fmovd %f4, %f2 |
There was a problem hiding this comment.
Maybe CHECK-DAG to allow for reordering
| return; | ||
| } | ||
|
|
||
| let total = arg.layout.size; | ||
| if total > in_registers_max { | ||
| arg.make_indirect(); | ||
| *total_double_word_count += 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Would you mind giving a couple of comments about what behavior this is implementing?
Fixes the 64-bit SPARC part of #115609 by replacing the current implementation with a new implementation modelled on the RISC-V calling convention code (SPARC ABI reference).
Pinging
sparcv9-sun-solaristarget maintainers: @psumbera @kulikjakFixes #115336
Fixes #115399
Fixes #122620
r? @workingjubilee