don't pass Complex<f16> in vector registers on i586 without SSE2

[folkertdev]

View all comments

Passing as an i32 is in line with how the other complex values are passed (e.g. Complex<f32> is passed as i64).

Clang just errors out on the use of _Float16, see https://godbolt.org/z/bd1vT6b9s.

r? @tgross35 or @RalfJung
cc @beetrees
Tracking issue: #154023

[rustbot]

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

[RalfJung]

So this file is for x86-32 only?

X86Sse2 is not used on x86-64.

View changes since the review

[folkertdev]

Yes? We have a separate compiler/rustc_target/src/callconv/x86_64.rs file, but there SSE2 is assumed.

[RalfJung]

Not sure I agree with this change. Before this PR, we're emitting an error if you try to pass Complex<f16> via extern "C", right? After this PR, the error goes away and we make up a non-standard ABI instead. I think I prefer the error.

[folkertdev]

The bigger picture here is that those libcalls will be called by the Mul impl for Complex<{float}>, and the behavior of Mul should not change depending on target features.

So then alternatively, on i586 in compiler-builtins we could define it with the sse2 target feature, and in core we could effectively inline the libcall so the ABI boundary disappears.

edit: my suggestion here is to only inline the libcall when there are ABI issues, otherwise the libcalls save some binary size and might in the future be more performant.

[RalfJung]

If we only need this ABI internally for our own libcalls, we could use the "Rust" ABI rather than a non-standard extension to the "C" ABI.

My main concern is that we shouldn't let people write or call extern "C" functions that pass Complex<f16> if the target feature is missing.

[beetrees]

On tier 2 x86_64-unknown-none, we currently let people write and call extern "C" functions with f32/f64 when the target features required by the ABI (SSE) are not available (x86-64, like x86-32, doesn't have any standard soft-float ABI AFAIK). Currently we pass those arguments in integer registers (using the classic non-standardised ABI of "whatever LLVM happens to do"). I feel like we should handle "primitive types that require unavailable registers in extern "C"" consistently, and I'm not sure what the best approach is, although I'm leaning towards handling it similar to vector types (especially when C compilers also don't support the types).

If we only need this ABI internally for our own libcalls, we could use the "Rust" ABI rather than a non-standard extension to the "C" ABI.

Seems like a good idea.

edit: my suggestion here is to only inline the libcall when there are ABI issues, otherwise the libcalls save some binary size and might in the future be more performant.

Surely having the libcall in core would be more performant as it would allow LLVM to inline the fast path?

[RalfJung]

we currently let people write and call extern "C" functions with f32/f64 when the target features required by the ABI (SSE) are not available (x86-64, like x86-32, doesn't have any standard soft-float ABI AFAIK).

As far as I am concerned, it has a standard soft-float ABI, it's whatever clang does. ;) But for this case, even clang thinks it's better to emit an error rather than invent a new ABI. I don't want us to even more reckless than clang when it comes to making up ad-hoc extensions of the "C" ABI.

[beetrees]

As far as I am concerned, it has a standard soft-float ABI, it's whatever clang does.

Fair enough. Upon further testing, on x86-64 Clang seems support soft-float passing and returning f32/f64 (but only when x87 is also disabled, even though the required registers are from sse). GCC seems to support passing them but not returning them (although it's possible I've just not found the right combination of flags).

[folkertdev]

If we only need this ABI internally for our own libcalls, we could use the "Rust" ABI rather than a non-standard extension to the "C" ABI.

No the idea is that by default you'd fall back to the version supplied by your libc, and only use the rust version when your libc does not provide one. Hence the binary size reduction.

That reduction seems pretty marginal though.

Surely having the libcall in core would be more performant as it would allow LLVM to inline the fast path?

We'd already inline the fast path (at least that is what clang does). Also I kind of mixed up two things

• the compiler-builtins repo has better infrastructure for benchmarking this sort of function
• in theory the libc implementation on a particular system could be more optimized than the fallback

[folkertdev]

I think I misunderstood the suggestion of using extern "Rust" earlier, I think this was the idea rust-lang/compiler-builtins#1303? Or at least I think that could work.

[RalfJung]

There's too much going on in that PR, but I see some pub extern "Rust" fn __rust so that looks about right. Basically we are defining our own builtins where we are in charge of the ABI and then we may as well use extern "Rust" rather than extern "C". We don't promise that you can mix compiler-builtins with stuff from other Rust versions, right?

[Jules-Bertholet]

My main concern is that we shouldn't let people write or call extern "C" functions that pass Complex<f16> if the target feature is missing.

No, we should trigger improper_ctypes. No reason to handle this case differently than all the others where we make up an ABI.

[RalfJung]

I think there's no reason to make up an ABI. The intended behavior of extern "C" is pretty clearly documented -- match the ABI of the default C compiler. It follows that we must error when we try yo pass types that the default C compiler would refuse to pass. Just because we mistakenly accepted some such types in the past does not mean we should continue perpetuating that mistake.

[tgross35]

As far as I am concerned, it has a standard soft-float ABI, it's whatever clang does. ;) But for this case, even clang thinks it's better to emit an error rather than invent a new ABI. I don't want us to even more reckless than clang when it comes to making up ad-hoc extensions of the "C" ABI.

Clang and GCC reject __int128, _Float16/__fp16, _Float128/__float128 on a huge number of platforms where LLVM supports it, and some of the accepted platforms are extensions of the platform ABI.

By that argument we should be emitting an error on f16, f128 with extern "C" on those platforms, since the types are still unstable. But that doesn't seem like a great option to me for those or for Complex—what is a user who hits the error supposed to do? Switch to extern "Rust" on specific platforms, opting the rest of the signature into the ABI instability that comes with it? That's not ideal if you're using extern "C" for cross-version compat.

IMO types that fall into this category should do the same thing as i128. That is, document the following:

• On unsupported platforms, the type has a "reasonably stable" extern "C" ABI that is generally not expected to change across versions.
• If the platform ABI docs get updated or the C compilers change, the ABI will be changed to match.
• improper-ctypes will be raised on these platforms

If anything, making some form of improper-ctypes deny-by-default seems preferable to a hard error.

[tgross35]

Passing as an i32 is in line with how the other complex values are passed (e.g. Complex<f32> is passed as i64).

I don't think this is accurate, they all seem to be passed indirectly so we should do the same https://c.godbolt.org/z/rG5a9rfsT. That is the case for returning though

I can see the argument for consistency with f32 but it would probably be best to treat it as a non-magic #[repr(C)] struct Complex { T, T } when there is no obvious fallback (so, memory return in this case). One advantage is this is something you can actually specify in C so extern "C" isn't totally unrealistic.

[folkertdev]

Bit-packing is also consistent with the small integer types?

    // I586:           define{{.*}} i32 @cplx_f16(ptr {{.*}} byval([4 x i8]) {{.*}})
    // I586:           define{{.*}} i64 @cplx_f32(ptr {{.*}} byval([8 x i8]) {{.*}})

    // I586:           define{{.*}} i16 @cplx_i8(ptr {{.*}} byval([2 x i8]) {{.*}})
    // I586:           define{{.*}} i32 @cplx_i16(ptr {{.*}} byval([4 x i8]) {{.*}})
    // I586:           define{{.*}} i64 @cplx_i32(ptr {{.*}} byval([8 x i8]) {{.*}})

But yeah not consistent with the plain struct (https://godbolt.org/z/zEPzj4vqv).

One advantage is this is something you can actually specify in C so extern "C" isn't totally unrealistic.

What do you mean by this?

[folkertdev]

IMO types that fall into this category should do the same thing as i128.

And this is our plan for stabilizing f16 as far as I know. We're waiting on LLVM 21 support being dropped, at which point this i586 ABI problem is the only blocker.

[RalfJung]

By that argument we should be emitting an error on f16, f128 with extern "C" on those platforms, since the types are still unstable. But that doesn't seem like a great option to me for those or for Complex—what is a user who hits the error supposed to do? Switch to extern "Rust" on specific platforms, opting the rest of the signature into the ABI instability that comes with it? That's not ideal if you're using extern "C" for cross-version compat.

What would extern "C" be good for if you can't call it from C because the compiler rejects that side of the code?

And this is our plan for stabilizing f16 as far as I know. We're waiting on LLVM 21 support being dropped, at which point this i586 ABI problem is the only blocker.

Is it? Not sure I like that... but anyway that's a topic for #131819.

[tgross35]

One advantage is this is something you can actually specify in C so extern "C" isn't totally unrealistic.

What do you mean by this?

If you're working with complex types in C on platforms that don't officially support it, the fallback is likely struct ComplexT { T re; T im; };. I think it's a minor advantage if this could interop with Rust code - but admittedly that's pretty niche.

It's a bit different but that's part of the motivation for Clang/GCC's recent changes to i128 on Windows: switching from xmm return to indirect means you can use __declspec(align(16)) struct i128 { long long x, y; }; with the MSVC compiler and it will be ABI-compatible with GCC's __int128.

By that argument we should be emitting an error on f16, f128 with extern "C" on those platforms, since the types are still unstable. But that doesn't seem like a great option to me for those or for Complex—what is a user who hits the error supposed to do? Switch to extern "Rust" on specific platforms, opting the rest of the signature into the ABI instability that comes with it? That's not ideal if you're using extern "C" for cross-version compat.

What would extern "C" be good for if you can't call it from C because the compiler rejects that side of the code?

It's still useful when you need some degree of ABI stability, Rust-to-Rust calls when a library is distributed or so. extern "crabi" may be the ideal thing for this case, but I don't think that has had progress in a few years.

It would feel rather weird to me if we warn but otherwise accept improper-ctypes for types that can be represented in C but are definitely incompatible (e.g. non-repr(C) structs) but draw the line for hard errors specifically at Complex<f16>. Additionally since, if the GCC maintainers don't change their mind, there will never be a C ABI for _Float16 or _Float16 _Complex without SSE. So there's nothing to be compatible with, but it's not footgun-prone because there is also nothing to be incompatible with.

[RalfJung]

It's still useful when you need some degree of ABI stability, Rust-to-Rust calls when a library is distributed or so. extern "crabi" may be the ideal thing for this case, but I don't think that has had progress in a few years.

I think that would be repeating the mistake we did with repr(C) and have to now painstakingly clean up -- the point of "C" is ABI-compatibility with the C toolchain, not arbitrary ABI stability.

It would feel rather weird to me if we warn but otherwise accept improper-ctypes for types that can be represented in C but are definitely incompatible (e.g. non-repr(C) structs) but draw the line for hard errors specifically at Complex.

I would call that "not repeating past mistakes".

[Jules-Bertholet]

People also use extern "C" for the abort-on-unwind guarantee. You can argue "we should have a dedicated language feature for that", but that doesn't exist right now.

Additionally, how would you propose to impose this restriction when the parameter type is a generic? Post-mono errors? Bury our head in the sand and pretend it can't happen (an approach we've used for several similar restrictions in the past)?

[RalfJung]

People also use extern "C" for the abort-on-unwind guarantee. You can argue "we should have a dedicated language feature for that", but that doesn't exist right now.

Yes, I would be arguing that. Using extern "C" for this is a hack, not something we should use as the basis for language design decisions.

Post-mono errors?

Yes. That's what we already do for SIMD types when required target features are missing. IMO it makes sense to treat __m128 and Complex<f16> basically the same if they use the same registers in the ABI. I don't see a good justification for treating them differently.

Bury our head in the sand and pretend it can't happen (an approach we've used for several similar restrictions in the past)?

Those are bugs (or best-effort lints), not "bury the head in the sand".

[tgross35]

I think that would be repeating the mistake we did with repr(C) and have to now painstakingly clean up -- the point of "C" is ABI-compatibility with the C toolchain, not arbitrary ABI stability.

I could tolerate rejecting everything f16 on i586 with extern "C", similar for any other unstable types, but I think:

1. It should be done consistently across all unstable types
2. Stable types should be considered (improper_ctypes deny-by-default?)
3. It needs an opt out (unsafe? allow? perma-unstable attribute? part of crabi?) because otherwise, compiler-builtins gets broken (function signatures with f16 and other types, where the other types have different extern "Rust" and extern "C" ABIs) and we kill other reasons one would use extern "C".
4. It needs lang input

For the time being, I don't see any reason some form of the change here shouldn't merge. We have a codegen fix for a very specific type that is otherwise not rejected - I'd much rather if that not be blocked on developing new lang policy.

[RalfJung]

The codegen change proposed here is inconsistent with what we do for SIMD types in the same situation. Is there any other situation where we go out of our way to invent a non-standard extension for the "C" ABI? I view this as precedent for moving in a direction that is IMO the wrong one, and that the relevant teams have not decided on. I can't see how this is a mere "fix", it is as much a policy decision as my proposal. In fact it is basically deciding to move away from my proposal, as IIUC the behavior without this PR is what I proposed (post-mono check to reject invalid use of this type in "C" ABI calls/definitions).

I would say accepting such code needs lang input much more so than rejecting it. Lang was involved in the SIMD type error that Complex<f16> is hitting; this PR is moving back on how we decided to treat types that need SIMD registers in the ABI. (We thought that would only affect SIMD types of course.)

[beetrees]

Currently, i586-unknown-linux-gnu uses the same C ABI as i686-unknown-linux-gnu - this is useful property e.g. for linking with (C) system libraries that may have been built with SSE2 enabled (or vice versa). This change makes a (small) break in that C ABI equivalence, in that C (or Rust) code built for i686 targets will use a different Complex<f16> C ABI than Rust i586 targets. For example, this would currently work on i586-unknown-linux-gnu:

unsafe extern "C" {
    // This function is defined in a C library that was built for `i686-unknown-linux-gnu`.
    safe fn returns_complex_f16() -> Complex<f16>;
}

// #[target_feature(enable = "sse")] should theoretically work but currently causes an LLVM error.
#[target_feature(enable = "sse2")]
unsafe fn i_checked_sse2_is_enabled() -> Complex<f16> {
    returns_complex_f16()
}

But won't work if we merge this PR, creating an ABI break. We could of course say "the C ABI on i586-unknown-linux-gnu is the same as the C ABI on i686-unknown-linux-gnu, except for Complex<f16> (and f16?)", but that feels like a level of definitionally complexity we generally want to avoid in ABI guarantees (and already do avoid with SIMD vector types). This is different than regular improper_ctypes as Complex<f16> can be used correctly in "C" ABI functions, just only when "sse" is enabled. That said, if there is interest in equalizing the treatment of regular improper_ctypes and types like Complex<f16> that are passed in vector registers, I think it should be in the direction of making the treatment of improper_ctypes stricter.

(The alternative here would be to declare that i586-unknown-linux-gnu has a different C ABI from i686-unknown-linux-gnu, but that would be inconsistent with how the target is generally used AFAIK.)

[rust-bors]

☔ The latest upstream changes (presumably #163583) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.