-
Notifications
You must be signed in to change notification settings - Fork 2.4k
cmake: don't auto vectorize with AltiVec on PPC #13321
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
base: main
Are you sure you want to change the base?
Conversation
Allowing GCC/Clang to auto-vectorize with AltiVec support breaks support for non-AltiVec compatible processors. Adding this option allows the existing AltiVec specific code to function as it always did, but prevents the compiler from inserting AltiVec instructions into arbitrary codepaths that SDL can't gate off via the existing CPUInfo code.
Hmm...I think this is probably okay, because it fits the spirit of what that piece of CMake code is trying to do, and historically we've had to deal with (Apple) computers that might or might not have AltiVec support. But for systems with guaranteed AltiVec units, we probably do want the compiler to use those instructions wherever it can, even just in basic for-loops outside of the blitters. Unlike the Mac, I assume most PowerPC systems are special case devices that you build specific binaries for. So I think merge this, but let's add a FIXME to maybe adjust this at some point. |
Is there a specific use case where this builds for a PPC processor and the code might run on a processor without Altivec instructions? |
Yes. Plenty of embedded cores do not support AltiVec, as well as any 'standard' PowerPC CPU that is older than a G4/7400 (G3/750, 60x, etc). |
But is SDL built with a toolchain that autovectorizes and then run on these platforms? |
What's proposed in this patch seems like a pretty big hammer. Does |
if(COMPILER_SUPPORTS_ALTIVEC) | ||
set(HAVE_ALTIVEC TRUE) | ||
set(SDL_ALTIVEC_BLITTERS 1) | ||
sdl_compile_options(PRIVATE "-maltivec") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If altivec is not always available on powerpc platforms, then this option must go.
Instead, we should use -maltivec
for only those sources that can contain altivec implementations and/or use the SDL_TARGETING("altivec")
macro (gcc supports it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes: We allow that macro for gcc >= 4.9, and it does seem to support it: https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Function-Attributes.html#Function-Attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line might be a deal-breaker:
In 32-bit code, you cannot enable AltiVec instructions unless -mabi=altivec is used on the command line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line might be a deal-breaker:
In 32-bit code, you cannot enable AltiVec instructions unless -mabi=altivec is used on the command line
Isn't that required in combination with -maltivec? (Or I remember wrong..)
Description
Allowing GCC/Clang to auto-vectorize with AltiVec support breaks support for non-AltiVec compatible processors. Adding this option allows the existing AltiVec specific code to function as it always did, but prevents the compiler from inserting AltiVec instructions into arbitrary codepaths that SDL can't gate off via the existing CPUInfo code.
Existing Issue(s)
None found.