switch out fpuPow variants for fpuExp2 #37
Conversation
There was a problem hiding this comment.
I dig it, thanks!
RE precision, I think we can address that in a separate patch, as it requires some exploration. The short answer is "I don't really know if we need extra precision" - I think my rule of thumb has been anything relating to phase/frequency should generally have double precision, but that may have been misguided and could certainly be revisited.
Right. It would be interesting to see if we could try to completely butcher the precision and see how bad it gets. I have a hunch that fp32 is plenty, but if we only compare float do double, it could be hard to tell in a proper way. I often try to remove bits until I can clearly tell the difference, and then add back a few bits for good measure. In this case we only want either 32-bit or keeping 64-bits, but perhaps knowing how many bits are actually useful would be good information? |
|
I think you're right for the most part. One area of concern is phase precision which I believe can be offset by global song time in seconds, and I think even after a couple minutes there can be noticeable precision loss in 32 bits. For LFO's/oscillators I imagine this being significant. Otherwise, for pitch (and most areas where we actually use pow) it would probably be fine to do that part of the calculation in 32 bit precision and convert to 64 before accumulating or something. |
Right, but you just need to do the bias in 64-bit, calculating the pitch in 64-bit shouldn't be needed for that, I think. |
|
Hmm, fails now with a missing symbol "___libm_sse2_log", which I can't reproduce locally.. |
|
I guess this means |
Calling Pow with a constant integer value is quite wasteful. Let's just turn this into simple arithmetics instead. To make potential future code a bit cleaner, let's add a few helpers.
kusma
force-pushed
the
remove-fpuPow
branch
from
2df064c
to
de1dda3
Compare
|
OK, let's try with some math-constants instead, should be less fragile. |
|
Yeah, that did the trick :) |
This these already returns 1.0 when x = 0.0, so no need to special case it.
kusma
force-pushed
the
remove-fpuPow
branch
from
de1dda3
to
00785b3
Compare
It turns out, we're don't really need the full flexibility of Pow(x, y). Instead, what we need is these:
The first two are trivially expressible as normal C++ floating-point multiplies. This seems to end up with exactly the same code-size, both before and after compression.
Number four can also trivially be expressed in terms of number three. This means we're left with an exp2-variant. And Exp2 is a bit easier to handle than Pow, especially due to less needed special-cases.
The end result is a saving of 36 bytes.
A follow-up question to this series might become: do we really need both float and double variants of these? But I guess @yupferris knows more about the rationale here...