-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[wasm] Fix build failure due to lack of _Float16 support #72891
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
[wasm] Fix build failure due to lack of _Float16 support #72891
Conversation
@swift-ci test WebAssembly |
3af22a2
to
32424ab
Compare
@swift-ci test WebAssembly |
|
include/swift/Runtime/SwiftDtoa.h
Outdated
/// Does this platform support _Float16 type? | ||
#ifndef SWIFT_STDLIB_HAS_FLOAT16_TYPE | ||
# if defined(FLT16_MAX) && !defined(__wasm__) | ||
# define SWIFT_STDLIB_HAS_FLOAT16_TYPE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SWIFT_STDLIB_HAS_FLOAT16_TYPE
is really misleading, since this has nothing to do with Swift's stdlib supporting Float16 or not. Please rename.
stdlib/public/core/Runtime.swift
Outdated
#if arch(wasm32) | ||
// Note that this takes a Float32 argument instead of Float16, because clang | ||
// doesn't have _Float16 on all platforms yet. | ||
typealias _Float16Storage = Float32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming here isn't really great either; this isn't really about "storage", but rather promoting to a type that's legal (in C) for the target.
I think I'd go with something like |
32424ab
to
d43d5c4
Compare
I was also thinking about better naming, thank you for your suggestions. It sounds reasonable to me.
Unfortunately, The Clang compiler header ( So we don't have a good way to detect whether the target support, so we may need to add |
@swift-ci test WebAssembly |
@swift-ci smoke test |
d43d5c4
to
6fea015
Compare
@swift-ci smoke test |
WebAssembly does not support _Float16 type, so we need to guard the use of the type. Unfortunately, Clang does not provide a good way to detect the support of _Float16 type at compile time, so just disable for wasm targets.
6fea015
to
c32c7f5
Compare
@swift-ci smoke test |
@swift-ci test WebAssembly |
expectEqual(Float16(bitPattern: bitPattern).debugDescription, expected) | ||
expectEqual(Float16(bitPattern: 0x8000 | bitPattern).debugDescription, "-\(expected)") | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does that happen? Surely Float32
can represent at least as many NaN
values as Float16
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Float32
can represent all NaN
patterns that can be representable as Float16
. However, sNaN values are converted to qNaN while roundtripping Float16
-> Float32
-> Float16
for argument passing on platforms that don't have native Float16 support.
The actual conversion happens in a builtin that is used by swift_float16ToString
to truncate Float32
-> Float16
: https://github.com/llvm/llvm-project/blob/1107b47dcd145518c7b811bf10e2b848782b0478/compiler-rt/lib/builtins/fp_trunc_impl.inc#L111
Therefore the check fails on WebAssembly unfortunatley.
check failed at /home/katei/ghq/work.katei.dev/swift-source/swift/test/stdlib/PrintFloat16.swift, line 151
first: "nan(0xff)" (of type Swift.String)
second: "snan(0xff)" (of type Swift.String)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a minimum reproducible code for the implicit bit pattern change.
#include <stdio.h>
#include <stdint.h>
void dump_bits(void *d, size_t size) {
unsigned char *ptr = (unsigned char *) d;
for (int i = size - 1; i >= 0; i--) {
unsigned int mask = 1 << (8 - 1);
unsigned char byte = ptr[i];
do {
putchar(mask & byte ? '1' : '0');
} while(mask >>= 1);
}
putchar('\n');
}
int main(void) {
uint16_t snan_bits = 32000;
union {
__fp16 f;
uint16_t i;
} u = { .i = snan_bits };
dump_bits(&u.f, sizeof(__fp16));
float casted = (float)u.f;
dump_bits(&casted, sizeof(casted));
__fp16 casted_back = (__fp16)casted;
dump_bits(&casted_back, sizeof(casted_back));
return 0;
}
$ clang -target wasm32-unknown-wasi check.c -o check.wasm
$ wasmtime check.wasm
0111110100000000
01111111101000000000000000000000
0111111100000000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, sNaN values are converted to qNaN while roundtripping
Float16
->Float32
->Float16
for argument passing on platforms that don't have native Float16 support.
I wouldn't claim to be an expert (@stephentyrone is the man to talk to here), but that sounds like a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert here too, but as far as I investigated, the root cause seems the LLVM default lowering behavior for half
type that passes around half
type as float
. Due to the LLVM lowering, Float16
is represented as f32
at Wasm level and sNaN values are converted to qNaN while extending bitPattern
of Float16(bitPattern:)
to Float32
. (I said the conversion happens while roundtripping f16 -> f32 -> f16 in printing, but I found it also happens more primitive Float16(bitPattern:)
casting)
I feel changing NaN payload pattern while bit-width extension seems not a strange behavior. According to x64 manual, some instructions that convert float to double (e.g. CVTSS2SD
) return qNaN when the source is sNaN1. And also Wasm does similar canonicalization for fp operations including truncation and promotion against f32/f642.
If we really want to guarantee bitwise equivalence between the given bitPattern and the resulted Float16's bitPattern, we have three options:
- Change compiler-rt's
__extendhfsf2
to hold NaN payload patterns - Change LLVM to enable
softPromoteHalfType
for Wasm targets to pass aroundhalf
type asi16
at Wasm level - Change
IRGen
to usei16
to representBuiltin.FPIEEE16
instead ofhalf
at LLVM IR level for such archs
But I'm not sure it's worth paying such effort only for sNaN Float16 on platforms without native support, but I'd like to hear @stephentyrone 's opinion.
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the correct behavior. A conversion is required to silence NaNs per 754.
Ah, that's a clang bug, then. |
WebAssembly does not support _Float16 type, so we need to guard the use of the type. Unfortunately, Clang does not provide a good way to detect the support of _Float16 type at compile time, so just disable for wasm targets.