-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LLVM miscompiles passing/returning half
on several backends by using lossy conversions
#97981
Comments
@llvm/issue-subscribers-backend-powerpc Author: None (beetrees)
Consider the following IR:
```llvm
define half @to_half(i16 %bits) {
%f = bitcast i16 %bits to half
ret half %f
}
define i16 @from_half(half %f) {
|
@llvm/issue-subscribers-backend-mips Author: None (beetrees)
Consider the following IR:
```llvm
define half @to_half(i16 %bits) {
%f = bitcast i16 %bits to half
ret half %f
}
define i16 @from_half(half %f) {
|
@llvm/issue-subscribers-backend-hexagon Author: None (beetrees)
Consider the following IR:
```llvm
define half @to_half(i16 %bits) {
%f = bitcast i16 %bits to half
ret half %f
}
define i16 @from_half(half %f) {
|
@llvm/issue-subscribers-backend-webassembly Author: None (beetrees)
Consider the following IR:
```llvm
define half @to_half(i16 %bits) {
%f = bitcast i16 %bits to half
ret half %f
}
define i16 @from_half(half %f) {
|
FYI: On PPC atleast you need to add
|
I remember seeing somewhere that s390x requires either zext or sext for all integer arguments and returns, since they have different ABIs and the generated code assumes they've been sign/zero extended properly. |
The handling here treats the ABI as implicitly adding an fpext/fptrunc in the argument/return lowering (or a strict_fpext in the strictfp case). LLVM semantics do not guarantee signaling nan quieting, but ideally there wouldn't be an implicit cast in the first place. This is a consequence of SelectionDAG's type legalization logic, where without a legal f16 types assumes promotion to float in all contexts. I agree this isn't the most sensible behavior. AMDGPU suffers the same issue for the antique targets without legal f16, which I've always found annoying. I think it would make more sense to treat it as i16, or if we really want to keep it in float registers, as the low bits (or high on big endian) that just happen to be stored in a float that need to be extracted as an integer. |
I've confirmed that the experimental C-SKY backend also appears to experience this issue. |
The LoongArch backend also appears to have this issue. |
CI in [1] seems to indicate that there are cases where the `f16` infinite recursion bug ([2], [3]) can make its way into what gets called during tests, even though this doesn't seem to be the usual case. In order to make sure that we avoid these completely, just unset `f16_enabled` on any platforms that have the recursion problem. This also refactors the `match` statement to be more in line with `library/std/build.rs`. [1]: rust-lang#729 [2]: llvm/llvm-project#97981 [3]: rust-lang#651
Consider the following IR:
As the only operation involved is a bitcast (in particular, there are no floating point type conversions in the LLVM IR), the values returned from
to_half
andfrom_half
should be bit-for-bit identical to the value passed to them as their only argument (just a different type). However, several targets pass/returnhalf
as afloat
. On these targets, LLVM will use the default float conversion builtins (such as__gnu_h2f_ieee
and__gnu_f2h_ieee
) to convert betweenhalf
andfloat
. The issue is that these builtins silence signalling NaNs which changes the NaN payload, meaning that the roundtrip ofhalf
->float
->half
is not lossless and causes the generated ASM to violate the semantics of LLVM IR. This miscompilation is similar to #66803.By inspecting the assembly LLVM emits, I've discovered that at least the following backends appear to be affected (in brackets are a specific target triple that I've checked):
csky-unknown-linux-gnuabiv2
with-mcpu=ck860fv -mattr=+hard-float
)hexagon-unknown-linux-musl
)loongarch64-unknown-linux-gnu
with-mattr=+f
): Fixed by [loongarch][DAG][FREEZE] Fix crash when FREEZE a half(f16) type on loongarch #107791mips64el-unknown-linux-gnuabi64
): Fixed by [MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199powerpc64le-unknown-linux-gnu
)sparc64-unknown-linux-gnu
)wasm32-unknown-wasi
): Already reported inwasm32
queitenshalf
signalling NaNs in some situations when passing/returning them #96438As none of these target's ABI specifications (that I've been able to find) specify how
half
should be passed (nor does Clang support_Float16
on any of these targets), and given that these targets are a subset of those affected by #97975, I'm filing this as a single issue as the ABI has probably been selected as an automatic default by LLVM rather than a deliberate choice by the backends. Ultimately there are two possible solutions: either fix LLVM to codegen lossless conversions betweenhalf
andfloat
when needed for the ABI (one way to do this would be with a new pair of builtins that don't silence signalling NaNs), or change the ABIs to pass/returnhalf
without converting it tofloat
(probably using the same ABI asi16
, but some targets might have better options).Related to #97975.
The text was updated successfully, but these errors were encountered: