x86-clang-support #120317

Ben-7 · 2024-04-05T18:09:02+02:00

Ben-7 commented

2024-04-05 18:09:02 +02:00

This pull request adds clang on Windows (clang-cl) support for Blender builds. There are some specific steps to get a build working.

Dependencies

All the normal dependencies for a Blender Windows build
clang-cl build tools

Build Steps
In a Visual Studio Developer Prompt, perform the following:

cd into the root blender directory
make update
Once finished, cd into lib\windows_x64
Apply the diff attached to this pull request. It makes a one line change in openvdb that causes an error during compilation. I've submitted a pr to fix this issue in openvdb.
Change directory back to root blender directory
mkdir build
cd build
cmake -G "Visual Studio 17 2022" -T "ClangCl" ..
Open the generated sln file and build the blender project
Build the INSTALL project

Test Coverage
With these instructions the build passes all 280 tests on my machine

Performance
Using the Classroom scene as a reference - available on the demo files page - I am seeing a 5% reduction in render time on both an AMD 7700x and Intel 14900k.

This pull request adds clang on Windows (clang-cl) support for Blender builds. There are some specific steps to get a build working. **Dependencies** * All the [normal dependencies](https://developer.blender.org/docs/handbook/building_blender/windows/) for a Blender Windows build * clang-cl build tools **Build Steps** In a Visual Studio Developer Prompt, perform the following: * cd into the root blender directory * `make update` * Once finished, cd into `lib\windows_x64` * Apply the diff attached to this pull request. It makes a one line change in openvdb that causes an error during compilation. I've submitted a pr to [fix this issue in openvdb](https://github.com/AcademySoftwareFoundation/openvdb/issues/1785). * Change directory back to root blender directory * `mkdir build` * `cd build` * `cmake -G "Visual Studio 17 2022" -T "ClangCl" ..` * Open the generated sln file and build the `blender` project * Build the `INSTALL` project **Test Coverage** With these instructions the build passes all 280 tests on my machine **Performance** Using the Classroom scene as a reference - available on the [demo files page](https://www.blender.org/download/demo-files/) - I am seeing a 5% reduction in render time on both an AMD 7700x and Intel 14900k.

rootnode_fix.diff

720 B

Iliya Katushenock added this to the Platforms, Builds Tests & Devices project 2024-04-05 18:14:45 +02:00

Xavier Hallade reviewed 2024-04-09 12:17:11 +02:00

source/blender/blenlib/intern/task_pool.cc Outdated

						
				@ -82,0 +81,4 @@

				  // TBB has a check in tbb/include/task_group.h where __TBB_CPP11_RVALUE_REF_PRESENT should evaluate to true as with

				  // the other MSVC build. However, because of the clang compiler it does not and we attempt to call a deleted constructor

				  // in the tbb_task_pool_run function. This check fixes this issue and keeps our Task constructor valid

				#if defined(WITH_TBB) && TBB_INTERFACE_VERSION_MAJOR < 10 || defined(_MSC_VER) && defined(__clang__)

Xavier Hallade commented

2024-04-09 12:17:11 +02:00

I assume it's fixed with newer versions of (one)TBB so it's best to keep this workaround to current version (2020u3 = TBB_INTERFACE_VERSION_MAJOR 11). I propose:
(defined(WITH_TBB) && TBB_INTERFACE_VERSION_MAJOR < 10) || (defined(_MSC_VER) && defined(__clang__) && TBB_INTERFACE_VERSION_MAJOR < 12)

I assume it's fixed with newer versions of (one)TBB so it's best to keep this workaround to current version (2020u3 = TBB_INTERFACE_VERSION_MAJOR 11). I propose: `(defined(WITH_TBB) && TBB_INTERFACE_VERSION_MAJOR < 10) || (defined(_MSC_VER) && defined(__clang__) && TBB_INTERFACE_VERSION_MAJOR < 12)`

Ben-7 commented

2024-04-10 20:09:02 +02:00

I've committed your recommendation. I attempted to build with the latest version of opentbb and your change in attempt to check if the latest version of tbb fixed the issue. Due to changes in namespaces - e.g. tbb::task becoming tbb::detail:d1::task - I needed to omit the usd library. Then ran into issues with openvdb and an undeclared symbol during linking.

However, since building with the latest opentbb headers and the check for TBB > 12 got all the way to linking without error I assume those errors are related to the libraries and old tbb versions as a dependency.

The USD library has open issues for opentbb here and here.

I've committed your recommendation. I attempted to build with the latest version of opentbb and your change in attempt to check if the latest version of tbb fixed the issue. Due to changes in namespaces - e.g. `tbb::task` becoming `tbb::detail:d1::task` - I needed to omit the usd library. Then ran into issues with openvdb and an undeclared symbol during linking. However, since building with the latest opentbb headers and the check for `TBB > 12` got all the way to linking without error I assume those errors are related to the libraries and old tbb versions as a dependency. The USD library has open issues for opentbb [here](https://github.com/PixarAnimationStudios/OpenUSD/issues/1471) and [here](https://github.com/PixarAnimationStudios/OpenUSD/issues/2964).

👍 1

Ben-7 marked this conversation as resolved

Ray molenkamp requested changes 2024-04-10 21:01:33 +02:00

Ray molenkamp left a comment

Haven't been able to test, but at first glance there are fewer changes than I feared

source/blender/makesdna/intern/CMakeLists.txt Outdated

						
				@ -78,3 +76,4 @@

				  ${dna_header_string_file}

				)

				# If the cmake compiler is not the specific match we're looking for (clang-cl compiling on windows)

Ray molenkamp commented

2024-04-10 20:55:16 +02:00

This smells a little, i made cleanup PR in #120490

Ben-7 commented

2024-04-16 06:59:46 +02:00

I've pulled in new commits from main including that PR and have dropped the commits affecting these lines.

Ben-7 marked this conversation as resolved

source/blender/makesrna/intern/CMakeLists.txt Outdated

						
				@ -234,0 +230,4 @@

				# If the cmake compiler is not the specific match we're looking for (clang-cl compiling on windows)

				# we can add these specific files. Otherwise we run into multiple symbol issues due to these 

				# same files being compiled in bf_intern_guardedalloc.lib

				if(NOT(CMAKE_C_COMPILER_ID MATCHES "Clang" AND CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC" AND WIN32))

Ray molenkamp commented

2024-04-10 20:55:38 +02:00

This smells a little, i made cleanup PR in #120490

Ben-7 marked this conversation as resolved

source/blender/nodes/geometry/CMakeLists.txt Outdated

						
				@ -289,1 +289,4 @@

				# If compiling with msvc clang we need to add the D_LIBCPP_VERSION define

				# so we don't run into tbb errors when compiling with lib

				if(MSVC_CLANG)

Ray molenkamp commented

2024-04-10 20:59:31 +02:00

if this is TBB specific, test if WITH_TBB is on before adding the flag

if this is TBB specific, test if `WITH_TBB` is on before adding the flag

Ben-7 commented

2024-04-16 06:56:18 +02:00

Added WITH_TBB check in 8556e4147f110.

Added `WITH_TBB` check in 8556e4147f110.

Ben-7 marked this conversation as resolved

source/creator/CMakeLists.txt Outdated

						
				@ -1127,1 +1127,3 @@

				    if(WITH_WINDOWS_RELEASE_STRIPPED_PDB)

				    # Skip install of stripped pdb if compiling with clang since there doesn't seem

				    # to be a pdbstripped version for clang-cl

				    if(WITH_WINDOWS_RELEASE_STRIPPED_PDB AND NOT(CMAKE_C_COMPILER_ID MATCHES "Clang"))

Ray molenkamp commented

2024-04-10 20:57:36 +02:00

why are you not using MSVC_CLANG here?

why are you not using `MSVC_CLANG` here?

Ben-7 marked this conversation as resolved

source/creator/CMakeLists.txt Outdated

						
				@ -1861,12 +1863,22 @@ endif()

				# `vcpkg` substitutes our libraries with theirs, which will cause issues when you you run

				# these builds on other systems due to missing DLL's. So we opt out the use of `vcpkg`.

				if(WIN32)

				  # Additional post build step to re-embed the manifest file

Ray molenkamp commented

2024-04-10 20:56:11 +02:00

Can you see if #111683 also resolves this problem without having a post build step

Ben-7 commented

2024-04-15 23:24:13 +02:00

Your PR resolves the problem without needing the post build step.

However, blender-launcher fails to compile properly since the manifest was left as a dependency for it - unsure if that's intentional and haven't tried building it with the msvc compiler. Removing that line fixed the issue for me.

Removed the post build and brought in the commit from your PR b44a7169c8117e937c74d1134c7a2f97ae93b8c9.

Your PR resolves the problem without needing the post build step. However, `blender-launcher` fails to compile properly since the [manifest was left as a dependency](https://projects.blender.org/blender/blender/src/commit/7f3149d1475a62d76db40b5261c72b88a4742c2a/source/creator/CMakeLists.txt#L333) for it - unsure if that's intentional and haven't tried building it with the msvc compiler. Removing that line fixed the issue for me. Removed the post build and brought in the commit from your PR b44a7169c8117e937c74d1134c7a2f97ae93b8c9.

source/creator/CMakeLists.txt Outdated

						
				@ -1868,2 +1879,3 @@

				  )

				  if(WITH_WINDOWS_RELEASE_PDB AND WITH_WINDOWS_RELEASE_STRIPPED_PDB)

				  # If compiling with clang-cl we skip PDBSTRIPPED. There's doesn't seem to be a switch for it currently

				  if(WITH_WINDOWS_RELEASE_PDB AND WITH_WINDOWS_RELEASE_STRIPPED_PDB AND NOT(CMAKE_C_COMPILER_ID MATCHES "Clang"))

Ray molenkamp commented

2024-04-10 20:57:44 +02:00

why are you not using MSVC_CLANG here?

why are you not using `MSVC_CLANG` here?

Ben-7 commented

2024-04-16 06:55:34 +02:00

Good question. Switched both checks using CMAKE_C_COMPILER_ID to MSVC_CLANG in 8556e4147f110.

Good question. Switched both checks using `CMAKE_C_COMPILER_ID` to `MSVC_CLANG` in 8556e4147f110.

Ben-7 marked this conversation as resolved

Ray molenkamp commented

2024-04-11 06:27:12 +02:00

What version of llvm/clang did you use for testing? Using my somewhat dated 16.0.0 it wasn't able to successfully build, i'll update to something more recent tomorrow

In file included from K:\BlenderGit\blender\source\blender\editors\interface\interface.cc:48:
In file included from K:\BlenderGit\blender\source\blender\editors\include\UI_abstract_view.hh:32:
In file included from K:\BlenderGit\blender\source\blender\editors\include/UI_interface.hh:11:
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\functional(1140,16): error: reinterpret_cast from 'const void *' to 'void (*)(bContext &)' casts away qualifiers
        return reinterpret_cast<const _Fx*>(this->_Target(typeid(_Fx)));
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
K:\BlenderGit\blender\source\blender\editors\interface\interface.cc(754,23): note: in instantiation of function template specialization 'std::function<void (bContext &)>::target<void (bContext &)>' requested here
  if (but->apply_func.target<void(bContext &)>() != oldbut->apply_func.target<void(bContext &)>())
                      ^
1 error generated.

edit: build with 17.0.6 and vs2022 headers completed successfully, single failed test but i suspect my tests folder may not have been in sync.

I ended up removing the manifest post build step, since ninja wasn't super fond of the syntax there and errored out on it, the resulting binary ran just fine though, maybe it's just a problem with the VS generator?

What version of llvm/clang did you use for testing? Using my somewhat dated 16.0.0 it wasn't able to successfully build, i'll update to something more recent tomorrow ``` In file included from K:\BlenderGit\blender\source\blender\editors\interface\interface.cc:48: In file included from K:\BlenderGit\blender\source\blender\editors\include\UI_abstract_view.hh:32: In file included from K:\BlenderGit\blender\source\blender\editors\include/UI_interface.hh:11: C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\functional(1140,16): error: reinterpret_cast from 'const void *' to 'void (*)(bContext &)' casts away qualifiers return reinterpret_cast<const _Fx*>(this->_Target(typeid(_Fx))); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ K:\BlenderGit\blender\source\blender\editors\interface\interface.cc(754,23): note: in instantiation of function template specialization 'std::function<void (bContext &)>::target<void (bContext &)>' requested here if (but->apply_func.target<void(bContext &)>() != oldbut->apply_func.target<void(bContext &)>()) ^ 1 error generated. ``` edit: build with 17.0.6 and vs2022 headers completed successfully, single failed test but i suspect my tests folder may not have been in sync. I ended up removing the manifest post build step, since ninja wasn't super fond of the syntax there and errored out on it, the resulting binary ran just fine though, maybe it's just a problem with the VS generator?

Ben-7 changed title from ~~x86-clang-support~~ to WIP: x86-clang-support

2024-04-15 20:40:19 +02:00

Ben-7 force-pushed x86-clang-support from 6daa45bdad to 5c3e37dbb3

2024-04-16 06:52:24 +02:00

Compare

Ben-7 commented

2024-04-16 07:09:05 +02:00

I also ran into that issue with an older version of clang. If it's necessary to have older clang versions functioning, I can push a fix for the casting errors.

For the manifest post build step: if you have the external manifest in the same directory as blender.exe all issues go away. Without the post build step, the manifest being copied into the binary was being malformed. Never figured out the reason why (perhaps related to an llvm resource compiler?) The malformed manifest caused the binary to produce side by side configuration error preventing startup.

With commit b44a7169c8117e937c74d1134c7a2f97ae93b8c9 the build process is working fine for blender.exe without re-embedding the manifest using mt.exe.

I also ran into that issue with an older version of clang. If it's necessary to have older clang versions functioning, I can push a fix for the casting errors. For the manifest post build step: if you have the external manifest in the same directory as `blender.exe `all issues go away. Without the post build step, the manifest being copied into the binary was being malformed. Never figured out the reason why (perhaps related to an llvm resource compiler?) The malformed manifest caused the binary to produce side by side configuration error preventing startup. With commit b44a7169c8117e937c74d1134c7a2f97ae93b8c9 the build process is working fine for `blender.exe ` without re-embedding the manifest using `mt.exe`.

Ray molenkamp commented

2024-04-17 01:52:04 +02:00

I'll do another cleanup pass #111683 so we can toggle it on/off on demand, in the mean time small improvement for your patch:

diff --git a/build_files/windows/configure_msbuild.cmd b/build_files/windows/configure_msbuild.cmd
index d4be40aff4f..6bf05d7448a 100644
--- a/build_files/windows/configure_msbuild.cmd
+++ b/build_files/windows/configure_msbuild.cmd
@@ -8,7 +8,7 @@ if "%BUILD_WITH_SCCACHE%"=="1" (
 )

 if "%WITH_CLANG%"=="1" (
-       set CLANG_CMAKE_ARGS=-T"llvm"
+       set CLANG_CMAKE_ARGS=-T"ClangCl"
 )

 if "%WITH_ASAN%"=="1" (

This bit of code will fix make.bat so you can just use make 2022 clang nobuild to generate the VS project, and you don't have to mess with cmake your self, the old llvm toolset can be replaced as it's from a time when llvm still shipped their own bindings given VS ships with a clang workload now, best to prefer that one. (also not entirely convinced llvm still ships bindings these days?)

I'll do another cleanup pass #111683 so we can toggle it on/off on demand, in the mean time small improvement for your patch: ``` diff --git a/build_files/windows/configure_msbuild.cmd b/build_files/windows/configure_msbuild.cmd index d4be40aff4f..6bf05d7448a 100644 --- a/build_files/windows/configure_msbuild.cmd +++ b/build_files/windows/configure_msbuild.cmd @@ -8,7 +8,7 @@ if "%BUILD_WITH_SCCACHE%"=="1" ( ) if "%WITH_CLANG%"=="1" ( - set CLANG_CMAKE_ARGS=-T"llvm" + set CLANG_CMAKE_ARGS=-T"ClangCl" ) if "%WITH_ASAN%"=="1" ( ``` This bit of code will fix `make.bat` so you can just use `make 2022 clang nobuild` to generate the VS project, and you don't have to mess with cmake your self, the old `llvm` toolset can be replaced as it's from a time when llvm still shipped their own bindings given VS ships with a clang workload now, best to prefer that one. (also not entirely convinced llvm still ships bindings these days?)

👍 1

Ray molenkamp requested changes 2024-04-18 17:07:26 +02:00

Ray molenkamp left a comment

I've landed #111683 which should resolve the manifest issues which can be removed from this PR

source/creator/CMakeLists.txt Outdated

						
				@ -1108,1 +1117,3 @@

				    if(WITH_WINDOWS_RELEASE_STRIPPED_PDB)

				    # Skip install of stripped pdb if compiling with clang since there doesn't seem

				    # to be a pdbstripped version for clang-cl

				    if(WITH_WINDOWS_RELEASE_STRIPPED_PDB AND NOT(MSVC_CLANG))

Ray molenkamp commented

2024-04-18 17:05:46 +02:00

I'm not sure what the parenthesis adds here, MSVC_CLANG should be an on/off style variable evaluating it first shouldn't change the results, if it's for readability AND (NOT MSVC_CLANG) is likely better? but even there i'm not convinced it's needed as it's not followed up by any additional conditions, what was the rationale for adding them?

I'm not sure what the parenthesis adds here, MSVC_CLANG should be an on/off style variable evaluating it first shouldn't change the results, if it's for readability `AND (NOT MSVC_CLANG)` is likely better? but even there i'm not convinced it's needed as it's not followed up by any additional conditions, what was the rationale for adding them?

Ben-7 commented

2024-04-22 20:26:15 +02:00

No particular reason. I've removed them in edd2f667b9

No particular reason. I've removed them in edd2f667b961e02d106bdc67822fc51cc0603a6c

Ben-7 marked this conversation as resolved

source/creator/CMakeLists.txt Outdated

						
				@ -1849,2 +1860,3 @@

				  )

				  if(WITH_WINDOWS_RELEASE_PDB AND WITH_WINDOWS_RELEASE_STRIPPED_PDB)

				  # If compiling with clang-cl we skip PDBSTRIPPED. There's doesn't seem to be a switch for it currently

				  if(WITH_WINDOWS_RELEASE_PDB AND WITH_WINDOWS_RELEASE_STRIPPED_PDB AND NOT(MSVC_CLANG))

Ray molenkamp commented

2024-04-18 17:05:57 +02:00

same as above

Ben-7 marked this conversation as resolved

Ben-7 force-pushed x86-clang-support from 3395e85656 to edd2f667b9

2024-04-22 19:50:12 +02:00

Compare

Ben-7 added 1 commit 2024-04-22 20:23:19 +02:00

clang-cl: added missing with_tbb for LIBCPP_VERSION define 90b7958ae8

Ben-7 commented

2024-04-22 20:25:19 +02:00

I've refactored removing all commits associated with the manifest embedding and updated to the current main. I also removed the parentheses for the NOT(MSVC_CLANG) cmake sections.

I can verify on my side, it continues to build and the binary opens successfully.

I've refactored removing all commits associated with the manifest embedding and updated to the current main. I also removed the parentheses for the `NOT(MSVC_CLANG)` cmake sections. I can verify on my side, it continues to build and the binary opens successfully.

Ben-7 changed title from ~~WIP: x86-clang-support~~ to x86-clang-support

2024-04-23 20:26:16 +02:00

Ray molenkamp referenced this issue from a commit

2024-04-24 15:59:59 +02:00

CMake: Windows: clang-cl fixes

Ray molenkamp merged commit 0136289cb6 into main

2024-04-24 16:00:00 +02:00

Ben-7 deleted branch x86-clang-support

2024-04-25 18:44:28 +02:00

Anthony Roberts referenced this pull request

2024-08-12 17:11:33 +02:00

Windows: Fix OpenVDB errors when compiling with clang-cl #126236