higan-emu / higan

higan is a multi-system emulator focused on accuracy, preservation, and configurability.
Other
1.19k stars 112 forks source link

Some code is GCC/Clang specific. #123

Open jchv opened 4 years ago

jchv commented 4 years ago

Some code uses GCC extensions that are not available on all compilers. This is unfortunate since the vast majority of code is portable. It would be nice to methodologically remove such code and, perhaps eventually, provide a build target for another compiler such as Microsoft Visual C++.

(This list will currently only be the one GCC specific code that is not currently gated by preprocessor conditionals. An attempt to compile with MSVC should garner some other interesting stuff.)

Known GCC-isms

(Should collect these; maybe even open separate issues.)

Dealing with Intrinsics

Clang has a handy preprocessor function __has_builtin which can be used to detect the availability of builtins. Unfortunately, GCC did not add support for this until the 10.x series, which means that in practice relying on this will disable intrinsics for most users.

Screwtape points out that it's possible compilers may already optimize the equivalent bit-twiddling hacks down to their optimal assembly equivalents, but unfortunately in practice even modern compilers do a surprisingly poor job detecting common idiomatic code. As an example, the byteswap code in nall generates poor assembly on most compilers, with GCC only able to optimize some of the cases, generating abysmal code for the 128-bit case. Trunk builds only improve this somewhat; Clang from trunk is finally able to optimize all of these basic byteswaps into ideal AMD64 assembly code, but the same can not be said for conventional __builtin_clz implementations.

If this were an autoconf based project, the obvious answer would be to use feature tests to check if the intrinsics exist and function as expected. Fortunately, this is not an autoconf project. Unfortunately, it being a pure GNU Make based application gives little opportunity to do any kind of feature detection. The byteswap implementations in nall/platform.hpp use the preprocessor conditional #if defined(COMPILER_CLANG) || defined(COMPILER_GCC) which is probably sufficient but is irritatingly imprecise.

Because of this, I suggest continuing to apply compiler-specific intrinsics using these crude preprocessor conditionals. If a more advanced build system is ever chosen, it would probably make sense to re-evaluate this, especially if said build system had a mechanism for doing feature checks like this.

Screwtapello commented 4 years ago

If I recall correctly, early versions of bsnes were MSVC-compatible, but it fell out of favour as byuu wanted to use C++ features MSVC didn't support yet, and I think something to do with profile-guided optimisation bugs. Once the build system was written in GNU Make, and made various assumptions about GCC-style compiler flags, re-adding MSVC support became highly unlikely... but it would be nice to at least have a list of GCCisms in the codebase.

jchv commented 4 years ago

I am not too concerned about the GNU Make stuff, it's more about making the code portable. A modern version of MSVC supports probably all of the needed standard features by this point.

That said, if nobody is strongly opposed to it, I do not believe a (best effort) MSVC project setup would be too hard to maintain, especially in a lower-momentum future. It only needs to support the Windows path anyways.

jeffythedragonslayer commented 1 year ago

I can't even build with gcc on Windows because this makefile spits out this:

GNUmakefile:10: *** "unsupported platform".  Stop.
Screwtapello commented 1 year ago

Are you building with gcc in MSYS2? Or some other gcc distribution?

jeffythedragonslayer commented 1 year ago

cygwin. Why do SNES people love MSYS2 so much? 🤔

Screwtapello commented 1 year ago

I think cygwin GCC builds executables that can only run in the cygwin environment, while MSYS2 gcc builds executables that run natively on Windows.

jeffythedragonslayer commented 1 year ago

That's a good reason.