make Complex ABI-compatible on sparc64 and powerpc64 - #161697
Conversation
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
| if is_ret { | ||
| let reg = Reg { kind: component.reg_kind(), size: component.size() }; | ||
| arg.cast_to(CastTarget::pair(reg, reg)); | ||
| } | ||
| return; |
There was a problem hiding this comment.
| if is_ret { | |
| let reg = Reg { kind: component.reg_kind(), size: component.size() }; | |
| arg.cast_to(CastTarget::pair(reg, reg)); | |
| } | |
| return; | |
| let reg = Reg { kind: component.reg_kind(), size: component.size() }; | |
| arg.cast_to(CastTarget::pair(reg, reg)); | |
| return; |
This seems to be relying on the PassMode::ScalarPair ABI matching the C calling convention for arguments, which (AFAIK) is something we generally avoid for non-Rustic ABIs. Any reason this can't unconditionally use PassMode::Cast?
(PassMode::Cast does currently emit less nice LLVM IR than PassMode::ScalarPair, but that's something that needs to be fixed more generally.)
There was a problem hiding this comment.
It almost works, except for Complex<i16> where I discovered this LLVM bug
Clang never runs into that because it just passes two separate arguments.
There was a problem hiding this comment.
| if is_ret { | |
| let reg = Reg { kind: component.reg_kind(), size: component.size() }; | |
| arg.cast_to(CastTarget::pair(reg, reg)); | |
| } | |
| return; | |
| if (component == Numeric::Int(Integer::I16)) { | |
| // FIXME: Using `PassMode::Cast` here would hit https://github.com/llvm/llvm-project/issues/218676. | |
| return; | |
| } | |
| let reg = Reg { kind: component.reg_kind(), size: component.size() }; | |
| arg.cast_to(CastTarget::pair(reg, reg)); | |
| return; |
I think this would work then, with the FIXME as a reminder to remove it once the bug is fixed (and/or if/when we stop using LLVM structs as the default option for PassMode::Cast arguments).
b5b4c02 to
9e30f0a
Compare
| //@ revisions: POWERPC64LE POWERPC64 AIX | ||
| //@ [POWERPC64LE] compile-flags: --target powerpc64le-unknown-linux-gnu | ||
| //@ [POWERPC64LE] needs-llvm-components: powerpc | ||
| //@ [POWERPC64] compile-flags: --target powerpc64-unknown-linux-gnu | ||
| //@ [POWERPC64] needs-llvm-components: powerpc | ||
| //@ [AIX] compile-flags: --target powerpc64-ibm-aix | ||
| //@ [AIX] needs-llvm-components: powerpc |
There was a problem hiding this comment.
I presume you chose these three for testing the three ABIs, so perhaps that could be more clear by calling them ELFV2, ELFV1 and AIX?
There was a problem hiding this comment.
well also endianness, and aix just because it's weird.
9e30f0a to
9e53b26
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Rollup of 14 pull requests Successful merges: - #162404 (`rust-analyzer` subtree update) - #161624 (diagnostics: Point closure trait errors at captured values) - #161697 (make `Complex` ABI-compatible on sparc64 and powerpc64) - #162182 (delay unexpected successful goal during ambiguity reporting) - #162328 (Allow overriding filecheck even if LLVM is built or downloaded) - #162367 (Use `reason` for tracked item diagnostics from `cfg_select!`) - #162381 (fix bare urls split text) - #162388 (std: fix set_permissions_nofollow on espidf and horizon) - #162319 (docs(core): correct ARMv8-M Baseline atomic CAS support) - #162341 (add regression test for packus_epi16 issue) - #162383 (Add a hint for using `nolimit` to the limiting error message) - #162384 (remove EnumSizeOpt) - #162390 (remove outdated comment in `UnsafeCell::raw_get` source) - #162397 (docs: Ask for ABI documentation in the platform support template)
Rollup merge of #161697 - folkertdev:complex-sparc64-powerpc64, r=JohnTitor make `Complex` ABI-compatible on sparc64 and powerpc64 tracking issue: #154023 Make the ABI for `Complex` match C for two slightly more involved targets. For Sparc64 the implementation matches GCC and Clang 24 and up. In llvm/llvm-project#215015 a bug with how small complex numbers are passed was fixed. For powerpc64 we match Clang exactly (and Clang is compatible with GCC for this target). cc @beetrees @Gelbpunkt
tracking issue: #154023
Make the ABI for
Complexmatch C for two slightly more involved targets.For Sparc64 the implementation matches GCC and Clang 24 and up. In llvm/llvm-project#215015 a bug with how small complex numbers are passed was fixed.
For powerpc64 we match Clang exactly (and Clang is compatible with GCC for this target).
cc @beetrees @Gelbpunkt