-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Support IEEE 754-2008 16-bits floats in bitstrings #2890
Conversation
931b023 to
ee2c4c0
Compare
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 for the PR! I think it's a pretty reasonable feature, we'll take it up on our next technical board meeting.
2be28a3 to
972bb4e
Compare
|
Sorry about the late reply, I missed the technical board meeting last month due to illness. We're interested in having this :) |
|
Yes!!! I will work on the test suite today! Thanks for the updates! :) |
972bb4e to
61f7ea5
Compare
|
Tests added and feedback addressed! |
95ec166 to
64fe9e7
Compare
64fe9e7 to
c455b32
Compare
|
The |
c455b32 to
ebf7736
Compare
|
@jhogberg done. I changed it to 8 which is what I did in the other similar suites. |
ebf7736 to
b79526d
Compare
erts/emulator/beam/erl_bits_f16.h
Outdated
| * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER | ||
| * DEALINGS IN THE SOFTWARE. | ||
| */ | ||
| static ERTS_INLINE float fp32_from_bits(uint32_t w) { |
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.
Sorry about the back and forth, but we need to change all instances of uint??_t in this file too. :(
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.
No problem! I should probably get a Windows machine next time I am working on this part of ERTS. :)
I have done the requested changes. I have also added a short notice at the top of this file (below the copyright disclaimer) to list all of the modifications we have done. This will make it easier to track changes or update the code in the future.
386d8c9 to
62d1e08
Compare
erts/emulator/beam/erl_bits_f16.h
Outdated
| exp_bits = (bits >> 13) & UINT32_C(0x00007C00); | ||
| mantissa_bits = bits & UINT32_C(0x00000FFF); | ||
| nonsign = exp_bits + mantissa_bits; | ||
| return (sign >> 16) | (shl1_w > UINT32_C(0xFF000000) ? UINT16_C(0x7E00) : nonsign); |
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.
[2021-02-03T10:53:10.718Z] erl_bits.o : error LNK2019: unresolved external symbol UINT32_C referenced in function erts_bs_get_float_2
[2021-02-03T10:53:10.718Z] erl_bits.o : error LNK2019: unresolved external symbol UINT16_C referenced in function fp16_ieee_from_fp32_value
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 @jhogberg! Which platform is it coming from? It seems those macros are part of the C99 standard, so I am wondering why they are not available. Although uint16_t is part of the C99 standard too.
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.
Follow up: do we support any 16bit platforms? My understanding is that this will matter only for those platforms, otherwise adding a U suffix would be enough.
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.
A relatively old version of MSVC that doesn't fully support C99 yet. We don't support any 16-bit platforms so it should be safe to use a U prefix.
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.
Updated! I also re-ran the added tests and they continue to pass.
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.
Thank you, it builds fine now :)
This format is often used with computer graphics and AI, also in cases where data is large enough that the decreased bandwidth and storage compensates the loss of precision. The _Float16 data type is defined in the C11 extension ISO/IEC TS 18661-3:2015. Recent compilers and platforms provide support for it and recent CPUs provide specific instruction sets for them too. In some, _Float16 is storage only, which is enough for our use cases. If _Float16 is not available, functions for conversion from float to half-floats are used, ported from https://github.com/Maratyszcza/FP16.
62d1e08 to
db21c04
Compare
| @@ -1132,17 +1178,21 @@ erts_new_bs_put_float(Process *c_p, Eterm arg, Uint num_bits, int flags) | |||
| #ifdef WORDS_BIGENDIAN | |||
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.
bs_construct_SUITE:fp16/1 fails on our big endian machine, can you look into that?
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.
Can you please post the failure if handy? I will try to figure out how to run this patch on a big endian machine. :)
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.
The first line of the test fails with {badmatch,false}, it doesn't say much more :/
I think the lines below are the culprit as they will always be zero when given an fp16, if I'm reading the code right you want to write the first 16 bits instead:
t[0] = a >> 24;
t[1] = a >> 16;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.
Ah, good point, thanks! I believe I have addressed your changes. I kept it as a separate commit for now as it also has some debugging information for the tests. If it works, then I can squash and we can discuss if we should keep the debugging info or 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.
Thanks, the fix looks a bit strange though: why set t[2] and t[3] for a 16-bit value? Shouldn't it be t[0] and t[1]?
| #ifdef WORDS_BIGENDIAN | |
| #ifdef WORDS_BIGENDIAN | |
| if (num_bits == 16) { | |
| t[0] = a >> 8; | |
| t[1] = a; | |
| } else if (num_bits >= 32) { | |
| t[0] = a >> 24; | |
| t[1] = a >> 16; | |
| t[2] = a >> 8; | |
| t[3] = a; | |
| if (num_bits == 64) { | |
| t[4] = b >> 24; | |
| t[5] = b >> 16; | |
| t[6] = b >> 8; | |
| t[7] = b; | |
| } | |
| } |
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.
@jhogberg of course your fix makes much more sense (or rather my fix made no sense whatsoever :D). I have applied it to both bigendian branches.
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, the tests pass and I'll merge this as soon as I can. There are some bureaucratic hurdles with importing the dependency that I hope will clear soon.
4dcf5fb to
8b9d974
Compare
|
Merged, thanks for the PR! |
This format is often used with computer graphics and
AI, also in cases where data is large enough that the
decreased bandwidth and storage compensates the loss
of precision.
The _Float16 data type is defined in the C11 extension
ISO/IEC TS 18661-3:2015. Recent compilers and platforms
provide support for it and recent CPUs provide specific
instruction sets for them too. In some, _Float16 is storage
only, which is enough for our use cases. If _Float16 is not
available, functions for conversion from float to half-floats
are used, ported from https://github.com/Maratyszcza/FP16.
I am sending this pull request to collect feedback. I will
work on a complete test suite if there is interest in moving
this forward.