Complex layout phase 2 - #159981
Conversation
There was a problem hiding this comment.
I think you'll also want tests for functions like safe fn complex_float_align2(_: c_float, value: Complex<c_float>) -> Complex<c_float>
also tests for properly handling complex arguments that could be split between registers and the stack (so the previous args take all but 1 fp or int reg), since different ABIs say to do different things (e.g. keep them together on the stack or split between registers and the stack)
There was a problem hiding this comment.
I've added a bunch more tests for that sort of scenario, and integrated my LLVM PRs (for mips, powerpc and sparc) so they now all match.
We should also run abi-cafe (and add complex support) to be fully sure, but for that we'd need to merge this first really.
This comment has been minimized.
This comment has been minimized.
3026f1f to
7fb70da
Compare
This comment has been minimized.
This comment has been minimized.
| // register alignment of 8 bytes. | ||
| // | ||
| // NOTE: clang uses a vector (e.g. <2 x f32>) here, but if we try that we run into | ||
| // ABI issues because vectors require the altivec target feature. |
There was a problem hiding this comment.
idk how you're getting that, from my testing clang 22 passes complex f32 via an indirect pointer and doesn't use any vector types:
https://clang.godbolt.org/z/676ajoT6a
assuming clang is correct, we should try to generate the same LLVM IR function signature, since that makes cross-language LTO work much better.
There was a problem hiding this comment.
assuming clang is correct
It is not.
- PowerPC ABI incompatibility with GNU on complex arguments llvm/llvm-project#56023
- [clang][PowerPC] Add flag to enable compatibility with GNU for complex arguments llvm/llvm-project#77732
- [PowerPC] make
_ComplexABI GCC-compatible llvm/llvm-project#208917
Being compatible with the standard C compiler on the platform (that determines the ABI of libcalls etc) seems more important than LTO for _Complex. Anyhow, I'm trying to fix the discrepancy in that bottom PR.
There was a problem hiding this comment.
This matters e.g. for
// main.c
#include <stdio.h>
#include <complex.h>
int main(void) {
volatile float re = -1.0f, im = 0.0f;
_Complex float s = csqrtf(re + im * I);
printf("%f + %fi\n", crealf(s), cimagf(s));
return 0;
}with
powerpc-linux-gnu-gcc -O2 main.c -o gcc.out -lm
# Use clang to compile, GCC as the linker.
clang --target=powerpc-linux-gnu -O2 -c main.c -o clang.o
powerpc-linux-gnu-gcc clang.o -o clang.out -lm
echo "gcc"
qemu-ppc -L /usr/powerpc-linux-gnu ./gcc.out
echo "clang"
qemu-ppc -L /usr/powerpc-linux-gnu ./clang.outreturns
gcc
0.000000 + 1.000000i
clang
4.186081 + 0.000000i
The clang result is nonsense.
There was a problem hiding this comment.
@programmerjake do you know anyone at IBM that could review llvm/llvm-project#208917? It's not getting much attention (equivalent PR for mips and sparc have already been merged).
There was a problem hiding this comment.
maybe lei137, since they reviewed a different PR I'm interested in? I'm not super familiar with who IBM has working on LLVM since I'm not an IBM employee.
There was a problem hiding this comment.
Sure, I just seem to remember you were working on powerpc related things, so thought you might have some personal connections. Lei is already reviewing some of my other PRs, e.g. llvm/llvm-project#209286. Hopefully they'll have time for this at some point, this kind of breakage is pretty bad...
7fb70da to
bc2336f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust does not emit `byval` here (also not for normal structs) while clang does.
so that we can specify more than one i32 of padding.
C `long double` is IEEE binary128 on 32-bit SPARC, and both GCC and Clang pass it `byval` and return it through an `sret` pointer (see Clang `SparcV8ABIInfo::classifyArgumentType` / `classifyReturnType`). rustc returned `f128` directly. The 32-bit SPARC ABI marks an indirect return by placing an `unimp <size>` word after the call, which the callee must skip by returning to `%o7+12`; LLVM keys that off the `sret` attribute. Without it the caller emitted the marker but the callee returned to `%o7+8`, landing on the marker word and dying with SIGILL.
GCC and Clang pass by-value aggregate arguments by reference on 32-bit SPARC: the caller makes a copy and passes its address. Clang's `SparcV8ABIInfo` falls back to `DefaultABIInfo::classifyArgumentType`, which makes any aggregate `byval`. rustc instead cast aggregates to a run of `i32` registers, so every by-value struct crossing the Rust/C boundary was corrupted, at any size. Aggregate returns were already indirect and stay that way. With aggregates and `f128` now all indirect, nothing reads the parameter array offset any more, so drop the offset tracking.
bc2336f to
f629989
Compare
tracking issue: #154023
Follow-up to #158885