Repository navigation
py/formatfloat: Fix exact int formatting on 32-bit mingw. - #10267
Merged
Merged
Conversation
Contributor
Author
|
This fixes the test failures seen in https://github.com/micropython/micropython/actions/runs/3726500144/jobs/6319986435 |
Contributor
Author
|
I don't know x86 assembly but here is the difference this change makes. Before (using `pow()`):After (using `powl()`):They look quite similar - neither calls any actual functions, so gcc must be optimising the |
|
Code size report: |
dlech
force-pushed
the
mingw32-float-format
branch
from
December 19, 2022 00:42
5232f70 to
fcb3d6d
Compare
|
Code size report: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10267 +/- ##
==========================================
+ Coverage 98.36% 98.49% +0.13%
==========================================
Files 159 155 -4
Lines 21093 20528 -565
==========================================
- Hits 20748 20220 -528
+ Misses 345 308 -37 ☔ View full report in Codecov by Sentry. |
Member
|
I'm not sure this is the right solution. What's probably happening is that it's using 80-bit float precision internally and that's making a slight difference to rounding in the uPy float algorithm. Or the compiler (with optimisation) ignores the IEEE standard; see Maybe there's a compiler option that can be used to fix this? Maybe we can compile floatformat.c with -O0 unconditionally on mingw32? Otherwise we can simply skip this test on mingw32 (see RUN_TESTS_SKIP in ports/windows/Makefile). |
Contributor
Author
We can actually just not optimize the one function. And I have confirmed it fixes the problem too. But since the problem is just with the pow() functions, it seemed like a bit overkill for such a large function compared to using powl() which just changes a few CPU instructions. diff --git a/py/formatfloat.c b/py/formatfloat.c
index fc1b2fe7f..1daee1360 100644
--- a/py/formatfloat.c
+++ b/py/formatfloat.c
@@ -98,6 +98,11 @@ static inline int fp_expval(FPTYPE x) {
return (int)((fb.i >> MP_FLOAT_FRAC_BITS) & (~(0xFFFFFFFF << MP_FLOAT_EXP_BITS))) - MP_FLOAT_EXP_OFFSET;
}
+#if MICROPY_FLOAT_IMPL == MICROPY_FLOAT_IMPL_DOUBLE && defined(__GNUC__) && defined(__MINGW32__) && defined(__i386__)
+// When optimizations are enabled using mingw's gcc, it breaks pow(double, double)
+// and gives wrong results.
+__attribute__((optimize("O0")))
+#endif
int mp_format_float(FPTYPE f, char *buf, size_t buf_size, char fmt, int prec, char sign) {
char *s = buf; |
Contributor
Author
|
I suppose another option could be to create a wrapper around |
Contributor
Author
I think I found the answer: https://lemire.me/blog/2020/06/26/gcc-not-nearest/ |
dlech
force-pushed
the
mingw32-float-format
branch
from
December 19, 2022 04:10
fcb3d6d to
847bc69
Compare
dlech
commented
Dec 19, 2022
dlech
force-pushed
the
mingw32-float-format
branch
3 times, most recently
from
December 19, 2022 04:59
38e455a to
7d96388
Compare
dpgeorge
reviewed
Dec 19, 2022
Member
There was a problem hiding this comment.
This needs a more substantial comment, eg "force gcc to use IEEE correct rounding when optimising float constants at compile time"
dlech
force-pushed
the
mingw32-float-format
branch
2 times, most recently
from
December 19, 2022 05:31
611b631 to
e58d368
Compare
Contributor
|
FYI for the msvc build we also use |
When compiler optimizations are enabled on the mingw version of gcc, we are
getting failing tests because of rounding issues, for example:
print(float("1e24"))
would print
9.999999999999999e+23
instead of
1e+24
It turns out special compiler options are needed to get GCC to use the SSE
instruction set instead of the 387 coprocessor (which uses 80-bit precision
internall).
Signed-off-by: David Lechner <[email protected]>
dpgeorge
force-pushed
the
mingw32-float-format
branch
from
February 5, 2024 03:06
e58d368 to
23342ef
Compare
tannewt
added a commit
to tannewt/circuitpython
that referenced
this pull request
May 3, 2025
Upgrade ESP-IDF v5.3.2 to v5.4.1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When compiler optimizations are enabled on the mingw version of gcc targeting 32-bit Windows, we are getting failing tests because of rounding issues, for example:
would print
instead of
We can work around the issue by using
powl()instead ofpow()inmp_format_float()on affected targets.