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

Unnecessary divss instructions #337

Closed
louis-langholtz opened this issue Aug 18, 2019 · 3 comments · Fixed by #338
Closed

Unnecessary divss instructions #337

louis-langholtz opened this issue Aug 18, 2019 · 3 comments · Fixed by #338
Assignees
Labels
Enhancement For suggestions or changes that enhance any part of the project and isn't a bug.

Comments

@louis-langholtz
Copy link
Owner

Just saw that g++9 does not do that automatically, there was still a divss instruction in there.

Oh wow!

I hadn't realized divss would get used instead of mulss even with maximum standards-compliant optimizations turned on. I recall having switched a bunch of places in the code base to using division to more directly express the intent of division under the assumption that the compiler would always convert these to using multiplication by the reciprocals.

Looking at things more closely on Godbolt reveals that it's divisions by floating point values that can't exactly be expressed reciprocally which are done via the divss instruction unless the -freciprocal-math optimization argument is added. Gcc, clang, and msvc all appear to convert any divisions by values that are exactly expressed reciprocally with mulss however at least in versions 9.2, 8.0.0, and 19.22.

Good news is this means there's places in the code that can be rewritten to be faster then. Bad news is that I don't recall all the places in the code where this will be.

Originally posted by @louis-langholtz in #331 (comment)

@louis-langholtz louis-langholtz self-assigned this Aug 18, 2019
@louis-langholtz louis-langholtz added the Enhancement For suggestions or changes that enhance any part of the project and isn't a bug. label Aug 18, 2019
@ninnghazad
Copy link
Contributor

Good candidates for a first round would be greps for / Radian. There are a bunch of these and i think most could be done with a search-replace-all.

@louis-langholtz
Copy link
Owner Author

/ Radian shouldn't result in the compiler generating any divss instructions with any optimization turned on. At least I certainly hope not.

If the C-preprocessor macro USE_BOOST_UNITS is defined, then / Radian converts an Angle value to a scalar, almost Real, value that's in units of radians. If this macro isn't defined, the compiler should always be recognizing / Radian as a divide by Real{1}, and then recognize that's a no-op.

I did take a look for anywhere / shows up and changed those places where the divisor is being used more than once or was constant and wasn't a one value. Changed the code to cache the result of the inverse and then multiply instead with that value as basically Erin does in Box2D. Just going through unit testing now and cleaning that up a bit.

@ninnghazad
Copy link
Contributor

Ah, ok. That clears up the confusion i had about it. Thought it looks noopish, but why do it then - but with the boost macro i get it. Thanks for shining a light on that for me.

I did take a look for anywhere / shows up and changed those places where the divisor is being used more than once or was constant and wasn't a one value. Changed the code to cache the result of the inverse and then multiply instead with that value as basically Erin does in Box2D. Just going through unit testing now and cleaning that up a bit.

looking forward to it. not that i feel playrho is slow - but faster is always welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For suggestions or changes that enhance any part of the project and isn't a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants