Skip to content

ports/unix/Makefile: make float() constistent across cpu architectures #4246

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

Closed
wants to merge 1 commit into from

Conversation

mksully22
Copy link

As discussed in forum thread https://forum.micropython.org/viewtopic.php?f=3&t=5332&p=30692#p30692

Because of the -Os optimization setting, the float() return values for ppc64le and x86_64 are calculated slightly differently because of the use of the Fuse Multiply-Add instructions. Adding the -ffp-contract=off compilation option will disable the use of the FMA instructions and make the values returned consistent across architectures(and generations) as desired under https://en.wikipedia.org/wiki/IEEE_754

@pfalcon
Copy link
Contributor

pfalcon commented Oct 17, 2018

as desired under https://en.wikipedia.org/wiki/IEEE_754

Surely, IEEE 754 standard doesn't specify that "use -ffp-contract=off switch for your compiler to make it compliant to me"? Or make me rephrase it differently: of thousands compilers and their versions out there, with how many did you test this change?

Hint: it won't take long to find one which breaks the build with this change. See #4182 for a recent case, and that's just a warning option, not a machine-specific flag!

@mksully22
Copy link
Author

mksully22 commented Oct 17, 2018

Sorry, I didn't mean to imply that IEEE_754 specifies the use of the fp-contract=off flag. Just pointing out the desire to return consistent results between cpu architectures. As it stands now the float_parse.py and float_parse_doubleprec tests in the micropython build fail on ppc64le when they encounter these slightly inconsistent results.

So if the inconsistencies are ok/expected for float() calculations on different cpu architectures then maybe we should delete/skip running the float_parse.py and float_parse_doubleprec.py tests?

@dpgeorge
Copy link
Member

Thanks for the report. I read the forum thread and it seems that indeed the problem is in parsing of floats. An easy way to check this without resorting to gdb is:

>>> import array
>>> bytes(array.array('d', [float('.' + '9' * 70)]))
b'\x00\x00\x00\x00\x00\x00\xf0?'

That will show the direct output of float parsing and not rely on float printing (which may have separate issues).

Unfortunately this is a tricky issue. I doubt that the C compiler is generating FP code that is against IEEE 754 (although it could be). One check would be to run a pure C program to parse the number using strtod and print out the bytes of the parsed double. Although that won't be definitive because the strtod function was likely compiled without -Os.

It could be that uPy's float parser does something "undefined" wrt FP calculations, and such an issue is only seen on ppc64.

To dig deep on this would really require stepping through mp_parse_num_decimal() on both x86-64 and ppc64 and see exactly where/when they start to disagree in their calculation. There must be a single line of C code in that function that is evaluated differently on these two architectures. Once that line is isolated a small reproducible test (in C) could be written that shows the discrepancy between the two architectures. Then it's a matter of seeing if the issue lies with the C compiler, or with the line of code being ill defined wrt FP.

@dpgeorge
Copy link
Member

As mentioned above, the core issue here is parsing floats. And using -ffp-contract=off doesn't really fix that, it just changes the behaviour so some floats now parse better but other floats parse worse. See #6024.

@dpgeorge dpgeorge closed this May 18, 2021
@dpgeorge
Copy link
Member

See also #5995.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants