x86-clang-support #120317

Merged
Ray molenkamp merged 9 commits from Ben-7/blender:x86-clang-support into main 2024-04-24 16:00:00 +02:00
Contributor

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

Dependencies

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.
Ben-7 added 6 commits 2024-04-05 18:09:13 +02:00
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
@ -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__)
Member

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)`
Author
Contributor

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).
Ben-7 marked this conversation as resolved
Ben-7 added 1 commit 2024-04-10 20:04:49 +02:00
Ray molenkamp requested changes 2024-04-10 21:01:33 +02:00
Ray molenkamp left a comment
Member

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

Haven't been able to test, but at first glance there are fewer changes than I feared
@ -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)
Member

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

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

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

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
@ -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))
Member

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

This smells a little, i made cleanup PR in #120490
Ben-7 marked this conversation as resolved
@ -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)
Member

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
Author
Contributor

Added WITH_TBB check in 8556e4147f.

Added `WITH_TBB` check in 8556e4147f110.
Ben-7 marked this conversation as resolved
@ -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"))
Member

why are you not using MSVC_CLANG here?

why are you not using `MSVC_CLANG` here?
Ben-7 marked this conversation as resolved
@ -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
Member

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

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

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 b44a7169c8.

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.
@ -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"))
Member

why are you not using MSVC_CLANG here?

why are you not using `MSVC_CLANG` here?
Author
Contributor

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

Good question. Switched both checks using `CMAKE_C_COMPILER_ID` to `MSVC_CLANG` in 8556e4147f110.
Ben-7 marked this conversation as resolved
Member

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
Author
Contributor

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 b44a7169c8 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`.
Member

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?)
Ben-7 added 1 commit 2024-04-18 00:55:30 +02:00
Ray molenkamp requested changes 2024-04-18 17:07:26 +02:00
Ray molenkamp left a comment
Member

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

I've landed #111683 which should resolve the manifest issues which can be removed from this PR
@ -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))
Member

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?
Author
Contributor

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
@ -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))
Member

same as above

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
Author
Contributor

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 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
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#120317
No description provided.