Add Complex type and repr(complex) - #158885
Conversation
rustbot
added
A-attributes
|
|
There was a problem hiding this comment.
Commit e4c802f shows how our LLVM IR representation differs from clang. For instance instead of <2 x float> we just use double, and sometimes we pass { double, double} instead of two separate double arguments.
Based on what I know that is perfectly compatible on an ABI-level, it's just textually different from clang.
|
Is it me or is the callconv implementation missing? I don't see any users of |
This comment has been minimized.
This comment has been minimized.
For x86_64 we only need custom behavior for f80. I've not yet implemented a target where it is needed, and I'd kind of prefer to do that in a follow-up |
| use FfiResult::*; | ||
|
|
||
| if !def.repr().c() && !def.repr().transparent() { | ||
| if !def.repr().c() && !def.repr().transparent() && !def.repr().complex() { |
There was a problem hiding this comment.
This likely needs some work, for example Complex<{integer}> or even weirder types like Complex<String> (if that's even allowed by the compiler) should not be considered FFI safe. Easily done in a followup however.
There was a problem hiding this comment.
Actually Complex<{integer}> might be de-facto fine under GCC and Clang, I'm not certain. It's a non-standard C extension though, so not sure what we want to do about that.
There was a problem hiding this comment.
I recommend we err conservatively and treat it as nonsense for now until anyone asks for it.
There was a problem hiding this comment.
Right, takes some more logic but we now only accept (transparently wrapped) float types, anything else triggers improper_ctypes.
There was a problem hiding this comment.
btw it is my intention to make Complex<{integer}> just be abi-compatible in practice, but whether we actually guarantee it can be discussed later.
Part 18: runs rust-lang/rust#158885 (Add Complex type / repr(complex), a novel feature) through the full pipeline. Critic emits 26 comments; base judge keeps 6 (6 accept / 20 reject). The 6 survivors are a real maintainer review; the rejects expose a NEW failure mode distinct from Part 17's knowledge blind spot — grounding failures, where the critic asks for what the diff already contains because it's pattern-matching the form of a review, not reading the hunk. Argues the judge's accept rate (8/12 cookbook vs 6/26 here) is a distribution meter: an unlabeled readout of how far out of depth the critic is on a given PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7300c0a to
a63e0f1
Compare
| // repr(complex) is internal, so this check is not as thorough as we'd require if it were | ||
| // user-facing. |
There was a problem hiding this comment.
Lmk if we should be more thorough here, but given that we never want to stabilize repr(complex), only Complex<T>, this seems (more than) adequate.
| use FfiResult::*; | ||
|
|
||
| if !def.repr().c() && !def.repr().transparent() { | ||
| if !def.repr().c() && !def.repr().transparent() && !def.repr().complex() { |
There was a problem hiding this comment.
Right, takes some more logic but we now only accept (transparently wrapped) float types, anything else triggers improper_ctypes.
| #[repr(complex)] | ||
| //~^ ERROR attribute cannot be used on macro calls | ||
| //~| ERROR `repr(complex)` is experimental | ||
| unreachable!(); |
There was a problem hiding this comment.
added this, it's already an error because there is no compatibility concern
This comment has been minimized.
This comment has been minimized.
a63e0f1 to
39d6ddf
Compare
This comment has been minimized.
This comment has been minimized.
39d6ddf to
faadfa3
Compare
rustbot
added
the
S-waiting-on-review
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_hir/src/attrs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing |
rustbot
removed
the
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
faadfa3 to
fac1794
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fac1794 to
a58b2d1
Compare
This comment has been minimized.
This comment has been minimized.
bc52ce8 to
9a26ba5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9a26ba5 to
c9a428c
Compare
|
Some changes occurred in compiler/rustc_attr_ir |
|
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. |
| ) | ||
| .emit(); | ||
| } | ||
| cx.check_target("(complex)", &AllowedTargets::AllowList(&[Allow(Target::Struct)])); |
There was a problem hiding this comment.
...hm.
...What happens if I put in repr(simd, complex), or repr(C, complex)?
We don't need to make this specifically a repr attribute.
There was a problem hiding this comment.
Yeah maybe a lang item is sufficient actually. Then the type could just be repr(C) and that would do the right thing for like Complex<String> etc.
There was a problem hiding this comment.
Yes, if you find that impl to work out similar to this then that would also be good.
| pub re: T, | ||
| /// The imaginary component. | ||
| pub im: T, |
There was a problem hiding this comment.
Huh, odd. I kind of would expect r and i or real and imaginary.
There was a problem hiding this comment.
There might be some value in matching the C function names real and imag, but I don't personally think it's compelling:
https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Complex-Data-Types.html
There was a problem hiding this comment.
I'm just following the RFC here, libs can re-litigate it later
| pub struct Complex<T> { | ||
| /// The real component. | ||
| pub re: T, | ||
| /// The imaginary component. | ||
| pub im: T, | ||
| } |
There was a problem hiding this comment.
Unsure if public fields is desirable here.
There was a problem hiding this comment.
Having to call a method just to get the real or imaginary part seems annoying
There was a problem hiding this comment.
Ah, OK, I didn't notice that on my scan.
| // Currently we only guarantee that repr(complex) on a Complex<{float}> is ABI-compatible. | ||
| #![feature(repr_complex)] | ||
| #![feature(f16, f128)] | ||
| #![deny(improper_ctypes)] | ||
| #![crate_type = "lib"] |
There was a problem hiding this comment.
Hmm. What sort of linting occurs with use of this in a by-reference fashion, if any?
| #[repr(C)] | ||
| //~^ ERROR conflicting representation hints | ||
| //~| WARN this was previously accepted | ||
| #[repr(complex)] //~ ERROR: `repr(complex)` is experimental | ||
| struct ConflictingRepr(u64, u64); |
There was a problem hiding this comment.
Ah so this happens at the "multiple reprs" level, my earlier question is about specifically the "one parens, two annotations" case.
9 participants
View all comments
tracking issue: #154023
See also #t-compiler > representing `_Complex`. The RFC states that the
Complextype should match the ABI of_Complexin C, but provides little detail on how to actually achieve this. We concluded thatrepr(complex)is probably the best approach. Like clang, that means we "desugar" the representation early:_Complexis not natively represented in LLVM IR.I've derived the ABI from
clangfor 28 targets, a decent sample of what we actually support. For now I'm only actually testingx86_64because (as you'll see) updating the tests is quite labor-intensive. If we're happy with this direction I can fill more of them in, likely in a separate PR.The libs side of the type is very bare-bones: I'm not as interested in that part. Others can fill in the various instances, methods, write tests for those, and improve the docs.
r? workingjubilee