Skip to content
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

COMMON: Fix reading and writing doubles from streams with older ARM toolchains #4015

Merged
merged 2 commits into from Jul 2, 2022

Conversation

ccawley2011
Copy link
Member

@ccawley2011 ccawley2011 commented Jun 18, 2022

Relevant issue: https://bugs.scummvm.org/ticket/13470

This fixes movement in Full Pipe on RISC OS. Remaining issues:

  • Doubles are the same as floats on the Dreamcast and Nintendo DS. On the DS, this is because of a define in portdefs.h that can easily be worked around, but on the Dreamcast I'm not sure how to ensure that doubles are read correctly.
  • Should compatibility be maintained in Hyperspace Delivery Boy and Versailles 1685 save files?

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jun 18, 2022

If I understand correctly, what you do will change how data is stored in save files for some old ARM architectures.
As a result, the new file format will be portable across platforms which the current one is not.
IMO, we don't have to try to maintain compatibility for this.

@zeldin
Copy link
Contributor

@zeldin zeldin commented Jun 18, 2022

So, is the idea here that floating point numbers should be read/written as IEEE double precision in some specific byte order to make the saves portable between ScummVM implementations? Moving saves to/from the Dreamcast is rather awkward because of the nonstandard memory cards and custom headers of the files, but it's not like it can't be done...

As for reading/writing IEEE doubles in the Dreamcast port, first of all the thing with the SH4 is that while it does support double precision floats, there is a mode switch that must be done between single-precision and double-precision math, and each double precision floating point register aliases two single precision floating point registers. Also double precision math is kind of slow. Because of this, the normal procedure in games and suchlike is to use the "single only" mode of GCC, in which "float" and "double" are both single precision IEEE floats. In this mode GCC will not emit any instructions dealing with hardware double precision, in order to avoid having to keep track of the mode switch (it assumes the FPU is always in "single precision" mode).

However, it's pretty simple to convert between single and double precision IEEE floating point numbers in software using integer instructions. If you don't need to handle NaN, infinities or denormalized numbers, it's just a matter of adjusting the offset of the exponent, and zero-padding or truncating the mantissa. This can work on the Dreamcast since the floating point numbers do follow the IEEE standard, even though the "double" type is single precision in "single only" mode.

From a more birds eye perspective though, there is no guarantee that float or double in a particular C implementation follows the IEEE standard at all. double can be 80-bit or whatever, for example. So for a truly portable implementation, you can't just assume that byteswapping will be enough. Converting between IEEE double precision encoding and the native floating point format can be done with conversions between integer and floating point (i.e. (int) and (double), not unions or memcpy) and the functions ldexp and frexp. I have done this for the programming language Pike, which features I/O of IEEE formatted floating point numbers as part of the core function library.

WRITE_BE_FLOAT32(b + 4, swap.u32.low);
}

inline double READ_FPA_FLOAT64(const void *ptr) {
Copy link
Contributor

@zeldin zeldin Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two just seem to be duplicates of READ_LE_FLOAT64 and WRITE_LE_FLOAT64. If having the same functions available under two different names is meaningful, I'd suggest having one of the names simply calling the other to reduce code duplication. After all, the functions are inline...

Copy link
Member Author

@ccawley2011 ccawley2011 Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function uses big endian word order and little endian byte order, so isn't identical to READ_LE_FLOAT64 or WRITE_LE_FLOAT64. Some background can be found here: libsdl-org/SDL#5607 (comment)

Copy link
Contributor

@zeldin zeldin Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I see now it's "PDP endian". Is it ever used?

Copy link
Member Author

@ccawley2011 ccawley2011 Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at the moment - I originally added it in case I needed to maintain save file compatibility, but it might still be useful in the future when reading from game data files.

@ccawley2011 ccawley2011 force-pushed the float-word-order branch 2 times, most recently from 1c52c4d to 18edea8 Compare Jun 20, 2022
@ccawley2011
Copy link
Member Author

@ccawley2011 ccawley2011 commented Jun 20, 2022

However, it's pretty simple to convert between single and double precision IEEE floating point numbers in software using integer instructions. If you don't need to handle NaN, infinities or denormalized numbers, it's just a matter of adjusting the offset of the exponent, and zero-padding or truncating the mantissa. This can work on the Dreamcast since the floating point numbers do follow the IEEE standard, even though the "double" type is single precision in "single only" mode.

From a more birds eye perspective though, there is no guarantee that float or double in a particular C implementation follows the IEEE standard at all. double can be 80-bit or whatever, for example. So for a truly portable implementation, you can't just assume that byteswapping will be enough. Converting between IEEE double precision encoding and the native floating point format can be done with conversions between integer and floating point (i.e. (int) and (double), not unions or memcpy) and the functions ldexp and frexp. I have done this for the programming language Pike, which features I/O of IEEE formatted floating point numbers as part of the core function library.

Unfortunately, this feels a bit beyond my capabilities, and I would like to backport this to the 2.6 branch, so it might be best to merge this as-is and address the Dreamcast issues in a follow up PR. The behaviour on Dreamcast should remain the same as before.

Copy link
Member

@lephilousophe lephilousophe left a comment

IMHO, we can ignore non-IEEE 754 architectures for now.
I am almost certain that we never supported one and even if we did, it's already broken.
This PR fixes bugs without introducing new ones.

The thing we may try to do before merging is adding support for converting from/to stored doubles and internal representation for Dreamcast and DS.
We must be cautious about it but it's definitely doable even with all special cases.

common/endian.h Outdated Show resolved Hide resolved
common/endian.h Outdated Show resolved Hide resolved
@zeldin
Copy link
Contributor

@zeldin zeldin commented Jun 27, 2022

I added a commit which fixes the case where double is IEEE-754 but 32-bit. I agree that adding support for non-IEEE-754 systems is probably overambitious at this juncture.

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jun 28, 2022

Great! Thank you very much.

@sev-
Copy link
Member

@sev- sev- commented Jul 2, 2022

Okay, let's do it. Thanks!

@sev- sev- merged commit 52cee62 into scummvm:master Jul 2, 2022
8 checks passed
@ccawley2011 ccawley2011 deleted the float-word-order branch Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants