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

Detect the ISA extension when building Embree and adjust its build configuration to match #95158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MonterraByte
Copy link
Contributor

@MonterraByte MonterraByte commented Aug 5, 2024

Resolves #49225. Resolves #91217.

This PR fixes building Godot with recent ISA extensions enabled (such as by using the -march= or /arch: flags for GCC/Clang and MSVC, respectively) when Embree support is enabled.

Normally, Embree's build system compiles the Embree code using controlled compiler flags so that the right instruction set is used. Godot's build system doesn't do this, it compiles Embree with the global compiler flags. When a compiler flag that enables, for example, AVX support is set, the Embree code will choose its AVX implementation (due to the __AVX__ preprocessor define being set by the compiler). This causes the build to break for two reasons:

  • Godot's build system defines EMBREE_TARGET_SSE2, but that doesn't match the code that was built. Specifically, it causes issues with this bit of code in kernels/bvh/bvh.cpp:

    #if !defined(__AVX__) || !defined(EMBREE_TARGET_SSE2) && !defined(EMBREE_TARGET_SSE42) || defined(__aarch64__)
      template class BVHN<4>;
    #endif

    which causes BVHN<4> to not be defined if the compiler supports AVX and EMBREE_TARGET_SSE2 is defined.

  • Godot's copy of the Embree library does not include some files used by the AVX implementation (specifically, the kernels/geometry/primitive8.cpp, kernels/bvh/bvh_intersector1_bvh8.cpp, kernels/bvh/bvh_intersector_hybrid4_bvh8.cpp, kernels/bvh/bvh_intersector_hybrid8_bvh4.cpp and kernels/bvh/bvh_intersector_hybrid8_bvh8.cpp files), causing more undefined reference errors.


To solve this, we need to explicitly disable newer ISA extensions for the Embree code through compiler flags. This PR goes a bit further by also letting the user pick which ISA extension (between SSE2, SSE4.2, AVX, AVX2 and AVX512) Embree should use at compile time as a build option, allowing Godot builds targeting newer CPUs to use a matching Embree build, instead of being stuck with SSE2. As such, it implements godotengine/godot-proposals#3932, but as a build option. This MR makes the build system detect which ISA extensions are enabled, and changes the Embree build configuration to match it.

(It does not compile multiple versions of Embree and let it pick the correct one at runtime, although this PR could as a base for implementing that.)

Tested on x86_64 Linux (GCC and Clang) and Windows (MSVC).

Notes:

  • I removed the -mxsave flag, as it seems unnecessary. Embree's build system doesn't set it, and Embree doesn't seem to use it (there are no references to __XSAVE__, or "xsave" in general).
  • I changed -mstackrealign to only be set in 32-bit MinGW builds, as, from what I gather, it's not needed for 64-bit code.

Known issues:

  • On MSVC, building Godot with /arch:AVX (or higher) (but building Embree with SSE2) still fails because, apparently, specifying /arch:SSE2 after /arch:AVX does not disable AVX support (the order of the paramters doesn't matter, MSVC always uses the highest one. I filed an MSVC bug report to fix that, please vote on it so it gets attention. (Alternatively, we'd need to filter out /arch flags, and I'm not sure how feasible that is.)

  • On MSVC, building Embree with AVX (or higher) fails because some parts of Embree really don't like being built with AVX turned on. Fixing this would require a lot of changes to the build system (we'd need to build some of the files without AVX and the others with AVX), which I don't want to do in this pull request.

    (Both of these issues are still present without this pull request, it doesn't introduce new issues AFAIK.)

@MonterraByte MonterraByte requested a review from a team as a code owner August 5, 2024 07:58
@MonterraByte MonterraByte changed the title Embree isa extensions Isolate the ISA extension used for building Embree and make it a build option Aug 5, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Aug 5, 2024
@Calinou
Copy link
Member

Calinou commented Aug 5, 2024

As such, it implements godotengine/godot-proposals#3932, but as a build option.

This PR only affects Embree though, not the autovectorization used to compile the rest of Godot (which is where most performance gains can be made).

I think ideally, both should be governed by the same option. It doesn't make much sense to target SSE2 for the engine but use AVX2 for Embree only, for instance (since the CPU requirement is already AVX2 at this point).

SConstruct Outdated Show resolved Hide resolved
@MonterraByte
Copy link
Contributor Author

I think ideally, both should be governed by the same option. It doesn't make much sense to target SSE2 for the engine but use AVX2 for Embree only, for instance (since the CPU requirement is already AVX2 at this point).

I agree. I just wasn't sure how to implement that. Unless we only allow the user to pick from a selection of preset instruction set options, we'll need a way to figure out what Embree instruction set is compatible with what the user specified.

I looked into it, and I think we can do so by running the preprocessor and checking if it defines support for the various instruction set extensions. For GCC, we can use gcc <compile flags> -dM -E - < /dev/null (also works on Clang). For MSVC, we can do something similar with /Zc:preprocessor /PD /EP. Then, we check the output and see if the various macros (__AVX__, __AVX2__, etc.) are defined, and adjust the Embree build flags accordingly.

I'm not very familiar with SCons, but I could try implementing this if it sounds like a better solution.

@MonterraByte MonterraByte force-pushed the embree-isa-extensions branch from efdbc98 to c422f84 Compare August 23, 2024 09:46
@MonterraByte MonterraByte changed the title Isolate the ISA extension used for building Embree and make it a build option Detect the ISA extension when building Embree and adjust its build configuration to match Aug 23, 2024
@MonterraByte
Copy link
Contributor Author

I removed the build option and made it be detected from the compiler flags used to build Godot.

@MonterraByte
Copy link
Contributor Author

The Linux Editor w/ Mono build failure seems to be network related.

@MonterraByte MonterraByte force-pushed the embree-isa-extensions branch 2 times, most recently from f2aba40 to 62e46cd Compare August 29, 2024 15:09
@MonterraByte
Copy link
Contributor Author

MonterraByte commented Aug 29, 2024

It seems to work fine with the newly-added clang-cl support.

(Just rebased for testing, didn't make changes to the PR.)

…nfiguration to match

Embree has specialized implementations for various ISA extensions to
improve performance. Its code uses preprocessor definitions to detect
which extension to use. Typically, its code is compiled multiple times,
with different extensions enabled, and, at runtime, it detects which one
should be used.

Godot's build system doesn't do this, it simply compiles Embree once,
for the base SSE2 instruction set, and uses that. However, it doesn't
properly guarantee that it is built correctly. If Godot is compiled for
a newer instruction set (such as by using `-march=x86-64-v3` with
GCC/Clang, or `/arch:AVX2` with MSVC), Embree will end up with mixed
code paths, and compilation will fail. (Additionally, Godot's copy of
the Embree source code omits files that are not used by SSE2 builds, but
are needed for AVX builds, which causes more build errors.)

This commit fixes the compilation issues by finding the highest Embree
ISA extension target that's compatible with the compiler flags set
by the user, and adjusting the build settings accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants