struct: Implement 'e' half-float format #12799
Conversation
|
Code size report: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12799 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 161 161
Lines 21156 21199 +43
=======================================
+ Hits 20816 20859 +43
Misses 340 340 ☔ View full report in Codecov by Sentry. |
|
This is extending a standard Python module with non-standard features. It would be better to stick with the standard functionality for maximum compatibility (or a subset of it to keep the code size down). As a workaround, it seems like you could use the Or this might be a job for |
I'm not sure what you mean, Python did add support for "e" in struct in version 3.6. @smurfix thanks for the implementation. Did you find a use/need for this? I'd be interested to know. |
🤦 how did I never notice that before |
|
Yep, I need this for CBOR compatibility. The CPython CBOR2 package does a little "if it fits in 16 bits without losing accuracy then use two bytes, not four" dance that can't be turned off (and why should it?); many for other languages also do this. I'll probably add a nice and small CBOR library (nothing like CBOR2 obviously, that's huge by now) to micropython-lib next. |
py/binary.c
Outdated
| @@ -175,6 +241,8 @@ mp_obj_t mp_binary_get_val_array(char typecode, void *p, size_t index) { | |||
| return mp_obj_new_int_from_ull(((unsigned long long *)p)[index]); | |||
| #endif | |||
| #if MICROPY_PY_BUILTINS_FLOAT | |||
| case 'e': | |||
| return mp_obj_new_float_from_f(mp_decode_half(((uint16_t *)p)[index])); | |||
There was a problem hiding this comment.
I don't think this line is needed, it would be used for array.array("e").
There was a problem hiding this comment.
I removed this, it's not needed.
| e = 0x1F; | ||
| m = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add test cases to test all paths of this function. I think it's fine to do this in the existing float_struct.py test, just loop over a handful of values to pack/unpack.
There was a problem hiding this comment.
I added comprehensive tests for these functions that cover all code paths.
|
@smurfix Which architectures do you need to support this on? On our arm targets, if you enable On Cortex-M0 this is implemented by libgcc: https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/arm/fp16.c#L64 but this appears to generate larger code than your version. On x86 you can also use There is no support on xtensa (even in libgcc). So, would it be acceptable to only add support for |
Well, given that xtensa is one of the platforms I need to run my code on … |
|
I think this is a useful addition, the But it would be good to support hardware 16-bit floats and the @smurfix are you able and willing to update this PR to work with |
|
Done; assuming that the existence of the |
@dpgeorge Another use case for half-float is dealing with |
py/binary.c
Outdated
| return mp_obj_new_float_from_f(fpu.f); | ||
| #else | ||
| return mp_obj_new_float_from_f(mp_decode_half(val)); | ||
| #endif |
There was a problem hiding this comment.
Instead of putting the "does target have float16 support" in these locations, I suggest having this line as simply:
return mp_obj_new_float_from_f(mp_decode_half(val));for both cases, and then logic elsewhere to provide the correct implementation of mp_decode_half(). Eg:
#ifdef FLT16_MAX
static inline float mp_decode_half(...) {
union { ... }
return fpu.f;
}
#else
...
#endifThis will fix a few build errors the current implementation has when float16 is available.
There was a problem hiding this comment.
I applied the above suggestion.
That doesn't look correct for ARM targets. If I enable $ echo | arm-none-eabi-gcc -mfp16-format=ieee -dM -E - | grep FLT16
#define __FLT16_HAS_QUIET_NAN__ 1
#define __FLT16_MIN_EXP__ (-13)
#define __FLT16_MAX_10_EXP__ 4
#define __FLT16_DECIMAL_DIG__ 5
#define __FLT16_DENORM_MIN__ 5.9604644775390625e-8F16
#define __FLT16_MIN_10_EXP__ (-4)
#define __FLT16_DIG__ 3
#define __FLT16_IS_IEC_60559__ 1
#define __FLT16_MAX_EXP__ 16
#define __FLT16_EPSILON__ 9.7656250000000000e-4F16
#define __FLT16_NORM_MAX__ 6.5504000000000000e+4F16
#define __FLT16_MAX__ 6.5504000000000000e+4F16
#define __FLT16_HAS_INFINITY__ 1
#define __FLT16_MANT_DIG__ 11
#define __FLT16_MIN__ 6.1035156250000000e-5F16
#define __FLT16_HAS_DENORM__ 1So I suggest to use |
2a701e7 to
c11f827
Compare
This commit implements the 'e' half-float format: 10-bit mantissa, 5-bit exponent. It uses native _Float16 if supported by the compiler, otherwise uses custom bitshifting encoding/decoding routines. Signed-off-by: Matthias Urlichs <matthias@urlichs.de> Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Using it increases code size by about 2k. Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
|
I have rebased this on latest master and made the following changes:
|
|
Thank you for your work on this! |
py/binary: struct: Support half-float.
This change implements the 'e' half-float format:
10-bit mantissa, 5-bit exponent.
The first patch uses "real" math to assemble and disassemble the half-float bits, while the second replaces the pesky math with mangling the half-float bits to real-float bits. This saves ~600 bytes of code size.
Closes #7467.