rustc_target: RISC-V Zfinx is incompatible with {ILP32,LP64}[FD] ABIs
#138872
Conversation
Because RISC-V Calling Conventions note that:
> This means code targeting the Zfinx extension always uses the ILP32,
> ILP32E or LP64 integer calling-convention only ABIs as there is no
> dedicated hardware floating-point register file.
{ILP32,LP64}[FD] ABIs with hardware floating-point calling conventions
are incompatible with the "Zfinx" extension.
This commit adds "zfinx" to the incompatible feature list to those ABIs
and tests whether trying to add "zdinx" (that is analogous to "zfinx" but
in double-precision) on a LP64D ABI configuration results in an error
(it also tests extension implication; "Zdinx" requires "Zfinx" extension).
Link: RISC-V psABI specification version 1.0
<https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0>
rustbot
added
S-waiting-on-review
|
To note, both GNU and LLVM toolchains check those conditions by making sure that This commit implements roughly the equivalent more directly on the ABI level but not on the extension level (i.e. without prohibiting coexistence I'm in an experiment how to implement those (and possibly complex implication rules on RISC-V) cleanly (while supporting all codegen backend). That's because, while prohibiting |
|
r? @workingjubilee (or reroll) |
Hrrrrm. cc @RalfJung @veluca93 I feel like we did build up some mechanisms for this somewhere, but maybe we really didn't and that would be new? |
|
Note that, while long-term solution is required sometime, I think this PR itself will not be blocked by those because:
FYI, I'll list some of the examples here but I think discussing about it is another topic.
|
|
I have no idea what this sentence means: |
There was a problem hiding this comment.
Please make the filename and test comment more specific. We already have tests ensuring that forbidden targets in an attribute are rejected, so if this is just "exactly the same test as before but with a different target feature", there's no point in such duplication.
Should we be rejecting |
|
I am confused about the status of Zfinx + F now. Does adding Zfinx somehow change the ABI of the function? Adding them to the check the way this PR does makes it sound like yes, it changes the ABI. OTOH some of the other comments above make it sound like it's not about the ABI at all, it's more that within a function, we can't have both Zfinx and F for some reason. That would seem to me like it's a different case and requires new machinery in rustc to check (or we can rely on LLVM checking this, since it's a per-function thing and not some global soundness property). |
As in the first comment, RISC-V Calling Conventions (a part of RISC-V psABI) states that:
So, I think this is valid. Give me some time to reply your other comments. |
|
@RalfJung ah, Zfinx + F is effectively a nonsensical combination because
There is a subset of float operations that thus exists in each context... the ones that require specifically F, for instance, are the "load from F register to X register" ones, but fused-multiply-add exists on both. The fmadd instruction is even encoded identically for each variant. The instruction's operands change meaning, however, for Zfinx: each bitfield in the encoding that addresses an F register now addresses the identically-numbered X register. |
|
@a4lg Can you add comments that record the fact that Zfinx is incompatible at the opcode level with F, as the two features use the same opcodes yet address different registers? Most features aren't "mutually exclusive" in this way. |
|
I see, thanks. So... what does LLVM do when you enable both? |
|
die, hopefully |
|
@a4lg Can you obtain a definitive answer instead of my hopes? |
|
Clang driver will print a error message https://github.com/llvm/llvm-project/blob/release/20.x/clang/test/Driver/riscv-arch.c#L534 LLVM will crash The check code in llvm: https://github.com/llvm/llvm-project/blob/release/20.x/llvm/lib/TargetParser/RISCVISAInfo.cpp#L724 Some tests: |
Oh lol.^^ I'm fine with landing this PR, but it's only a half-way solution then. Could you file an issue tracking adding proper rustc checks here so one can never hit the LLVM error? |
Because RISC-V Calling Conventions note that:
{ILP32,LP64}[FD]ABIs with hardware floating-point calling conventions are incompatible with theZfinxextension.This commit adds
"zfinx"to the incompatible feature list to those ABIs and tests whether trying to add"zdinx"(that is analogous to"zfinx"but in double-precision) on a LP64D ABI configuration results in an error (it also tests extension implication;ZdinxrequiresZfinxextension).Links: RISC-V psABI specification version 1.0
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/v1.0/riscv-cc.adoc#named-abis
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0
Related:
#[target_feature]#44839(
riscv_target_feature)@rustbot label +T-compiler +O-riscv +A-target-feature