-
Notifications
You must be signed in to change notification settings - Fork 157
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
Draft for adding IEEE half-precision floating-point type. #172
Conversation
Thanks for the proposal. First, I think this belongs in the C API doc (not the ELF psABI). Although I guess we have a bunch of C type definitions here already (not sure I agree with that decision either). Second, I'm opposed to this proposal. The observation that We debated this at length on internal SiFive channels and I believe we established that EDIT: looking more closely at your proposal, I see that you specify promotion to |
Could you add a description about RISC-V are using IEEE 754-2008 format for |
I prefer
[1] IEC 60559 http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2405.pdf |
Do you mean create a new section
Yes, it's different to ARM's
Okay |
address kito's comment and fix typo. |
To expand on Kito's comment, IEC 60559 defines _Float16 as an interchange type. It is meant to be used as a type for exchanging FP vales between two computers that may have different FP implementations. So _Float16 is always a 16-bit IEEE FP type, it never promotes, and may not be usable for native arithmetic if the hardware doesn't support exactly the right FP semantics. Thus it isn't equivalent to float. Use of this may cause problems. __fp16 is intended to work the same as float, except 16-bits instead of 32-bits, but isn't formally specified, so there may be issues with that. There is an ISO C proposal to add a "short float" type to be used for 16-bit FP arithmetic. This includes adding a printf format specifier for short float values. There is no implementation that I'm aware of, and I don't know the status of the proposal. I think short float would be the best approach, except it doesn't exist yet. I think __fp16 is the second best approach, as it is usable today, for both C and C++ in both gcc and clang. _Float16 isn't supported in gcc C++ and because it never promotes it can't be used with stdarg functions like printf. We would also be at risk of violating a standard if we don't design the RISC-V FP support exactly the same way that the standard requires. If someone wants bfloat16 instead of ieee fp16, then you absolutely can not use _Float16 for that, but you can use __fp16 (and short float if it actually existed). |
Actually it is scanf that gets a new format code for short float. printf doesn't need one because short float promotes. |
I don't see how these problems are relevant: this proposal is explicitly about adding IEEE binary16 support to the C language, and the RISC-V Zfh-extension implements IEEE binary16. I agree there's a risk in effectively adopting a not-yet-approved C-language extension. But the IEEE spec is ratified/published, and Zfh is conformant. My opinion is that if Zfh is not present, then the compiler should call soft-float routines, just like it currently does if you use We keep coming back to I agree there's a bigger issue concerning supporting non-IEEE types, like BF16, which may be simultaneously present with binary16 on a RISC-V machine. But I think that's out of scope here. |
_Float16 is a distictly different kind of type from float and double. float and double are standard FP types. _Float16 is an interchange FP type. The compiler does not handle them the same way. We run the risk of user confusion if we recommend _Float16 in places where people are accustomed to using float or double. We also accidentally run the risk of violating the proposed standard if we get something wrong. _Float16 is defined by a proposed extension to the ISO C standard. I'm not aware of an equivalent ISO C++ extension. The GCC support for _Float16 suggests that the C++ proposal doesn't exist yet. Trying to use something in C++ that hasn't been formally defined yet may lead to trouble. I think both __fp and _Float16 have obvious problems. And that "short float" is the only good solution long term, but there is no implementation as yet. In the short term, since __fp isn't part of any standard, we run no risk of accidentally violating a standard if we use it, and hence I think it is the best solution. The workaround for printf _Float16 is an explicit cast to float. The "short float" proposed type is a standard FP type, and hence works for printf as it gets promoted. __fp works the same as short float so is OK. Printf isn't a big problem, it is just an example to show that _Float16 does not work the same as standard FP types, and that can potentially lead to user confusion. Maybe the best solution is to not define a standard C/C++ interface for the zfh extension at this time, and wait until the C and C++ standards have better support for 16-bit float values. |
After digging into clang implementation, |
Just read the spec and gcc implementation again, binary interchange floating types like
I think that means promote to float, double or long double before evaluation is legal behavior to |
Oh, one thing we missed is we should define the C++ name mangling for |
After discussion with clang and gcc community, we think
[1] https://lists.llvm.org/pipermail/cfe-dev/2021-March/067850.html |
riscv-elf.md
Outdated
The half precision floating-point value can be passed as function argument and | ||
returned by a function. |
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.
This is already covered by the general "Floating-point reals ...' paragraph above, no?
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, you are right.
After discussion with clang and gcc community, we choose C ISO/IEC TS 18661-3 `_Float16` rather than redefine a different behavior type `__fp16` in RISC-V. 1. ARM folks are working on deprecating `__fp16`. [1] 2. In Clang, `_Float16` is available in both C and C++ mode. [2] 3. In GCC, `_Float16` is only avaiable in C mode but GCC folks think making it available in C++ as GCC extension is doable. [3] [1] https://lists.llvm.org/pipermail/cfe-dev/2021-March/067850.html [2] https://gcc.gnu.org/pipermail/gcc/2021-March/234981.html [3] https://gcc.gnu.org/pipermail/gcc/2021-March/234972.html
Fixed! Thanks!! |
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.
Thanks, LGTM!
I suppose this is less controversial changes since it's defined same as other targets and also has been implemented on clang and GCC, so we'll merge that once @jrtc27 give an explicit LGTM. |
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.
Oops, I meant to approve this but didn't. PR policy requirements are met; Clang support has even been merged by now (llvm/llvm-project@77bb82d).
In order to provide the broadest level of compatibility across language versions
and compilers in libraries and programs, we adopt the
__fp16
rather than_Float16
as half-precision floating-point types. For example in the
printf
case, there isno format specifier for the C language extension type half-precision floating-point.
In the case of passing a
_Float16
to%f
or%d
the value will be ignored andsilently fail, and no value will be formatted. In the case of
__fp16
, there is animplicit cast to double, therefore the value will be formatted. So if you pass
_Float16
to printf, you won't be able to print it, as there is no printf format codefor half-precision floating-point types. If you pass __fp16 to printf, it will print
fine, as it will promote to double.
For implementation it in LLVM,
In RISCV, set Opts.NativeHalfArgsAndReturns = 1;
When
zfh
extension is enabled, set Opts.NativeHalfType = 1;ref: