Fix out-of-the-box vs2019 support for x86 #60

yupferris · 2021-06-13T21:50:28Z

See commits for further explanation.

Fixes #59.

MSVCRT/msvcrt.def

MSVCRT/mainCRTStartup.c

WaveSabrePlayerLib/CMakeLists.txt

WaveSabreCore/CMakeLists.txt

Also make all internal storage/arithmetic related to it use doubles. The main motivation for this is that it allows us to work around some of the issues identified in #59, as it avoids 64-bit integer arithmetic altogether. doubles have 52 significand bits, so by keeping track of bytes read by DirectSound, it should be accurate up to 7 million hours or something (as opposed to about 6 hours with 32 bit integers, and even that is a probably a bit overkill actually, except for perhaps a musicdisk and perhaps some extra bits for headroom).

There was a typo here, so the expression was always false, and removing it is equivalent. Additionally, we actually _want_ this definition for x86 builds as well as x64 (verified with vs2019 at least; I'm sure earlier versions behave similarly), which is yet another reason to remove it.

yupferris · 2021-06-14T11:31:10Z

OK, I think I've got a solution I'm happy with now.

yupferris · 2021-06-14T11:54:31Z

I guess we can also stuff phase into the union and perhaps make exponent extraction "simpler", or something. Probably just nitpicking at this point though.

WaveSabreCore/src/Helpers.cpp

yupferris · 2021-06-14T18:46:55Z

Well we sure took the scenic route, but I think we got where we wanted to 😅

WaveSabreCore/src/Helpers.cpp

We had these defined in two places, and the definitions were actually divergent. The table size ended up being determined by the size definitions in the cpp file, however, all indexing/usage of the table was determined by the Helpers members. Luckily, this actually behaves exactly as intended, but has the consequence that the table occupies twice as much memory as it should have (and the second half was never touched). This is super nasty, though, so let's clean it up.

The primary motivation is that the previous code relied on a 64-bit left shift, which with MSVC 14.2 and higher targeting x86, would generate code that relied on a function from MSVCRT.lib (not .dll) that we don't implement. By constraining the input range to [1, 2) using 1 + x - floor(x), we guarantee that the phase's exponent is always 0, so we can just take the significand bits without shifting them by the exponent. There's still a right-shift to isolate the significand, but this is by a constant factor, and is thus compiled without any issues. Additionally, because we limit the range this way, we also don't need to take advantage of cosine's symmetry about the y axis, so we remove the abs call as well. This new implementation has almost identical error compared to the previous, and is actually about 10% slower, but it works out-of-the-box on both x86 and x64 in the MinSizeRel configuration without any additional changes, and should still be quite portable.

This is used in more places in the code, especially critical places (eg. the Falcon oscillators), so this should ideally be the faster one, especially now that we don't need to take advantage of cos' symmetry about the y axis.

kusma · 2021-06-15T14:55:47Z

Looks most excellent to me! 🧐

yupferris force-pushed the x86-link-debugging branch from 863b0fd to 55029f8 Compare June 13, 2021 21:52

yupferris mentioned this pull request Jun 13, 2021

Fix out-of-the-box vs2019 support for x86 #59

Closed

yupferris force-pushed the x86-link-debugging branch 3 times, most recently from 99d2ce2 to 0342ae2 Compare June 13, 2021 22:43