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
7 changed files with 41 additions and 11 deletions

View File

@ -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" (

View File

@ -122,11 +122,23 @@ elseif(CMAKE_COMPILER_IS_GNUCC OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang"))
check_cxx_compiler_flag(-mavx2 CXX_HAS_AVX2)
# Assume no signal trapping for better code generation.
set(CYCLES_KERNEL_FLAGS "-fno-trapping-math")
# Avoid overhead of setting errno for NaNs.
string(APPEND CYCLES_KERNEL_FLAGS " -fno-math-errno")
# Let compiler optimize 0.0 - x without worrying about signed zeros.
string(APPEND CYCLES_KERNEL_FLAGS " -fno-signed-zeros")
# We need to omit or modify specific flags to pass through clang-cl to prevent
# unknown flag errors
if (WIN32 AND MSVC)
# Pass clang flags directly to clang otherwise. Clang-cl doesn't recognize
# these flags by default
set(CYCLES_KERNEL_FLAGS " /clang:-fno-trapping-math")
# Avoid overhead of setting errno for NaNs.
string(APPEND CYCLES_KERNEL_FLAGS " /clang:-fno-math-errno")
# Let compiler optimize 0.0 - x without worrying about signed zeros.
string(APPEND CYCLES_KERNEL_FLAGS " /clang:-fno-signed-zeros")
else ()
set(CYCLES_KERNEL_FLAGS " -fno-trapping-math")
# Avoid overhead of setting errno for NaNs.
string(APPEND CYCLES_KERNEL_FLAGS " -fno-math-errno")
# Let compiler optimize 0.0 - x without worrying about signed zeros.
string(APPEND CYCLES_KERNEL_FLAGS " -fno-signed-zeros")
endif()
if(CMAKE_COMPILER_IS_GNUCC)
# Assume no signal trapping for better code generation.

View File

@ -676,7 +676,7 @@ extern bool BLI_memory_is_zero(const void *arr, size_t arr_size);
*/
#define BLI_ENABLE_IF(condition) typename std::enable_if_t<(condition)> * = nullptr
#if defined(_MSC_VER)
#if defined(_MSC_VER) && !defined(__clang__)
# define BLI_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
#elif defined(__has_cpp_attribute)
# if __has_cpp_attribute(no_unique_address)

View File

@ -78,7 +78,10 @@ class Task {
other.freedata = nullptr;
}
#if defined(WITH_TBB) && TBB_INTERFACE_VERSION_MAJOR < 10
// 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__) && TBB_INTERFACE_VERSION_MAJOR < 12)
Ben-7 marked this conversation as resolved Outdated

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)`
Outdated
Review

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).
Task(const Task &other)
: pool(other.pool),
run(other.run),

View File

@ -133,7 +133,12 @@ if(WITH_GTESTS)
blender_add_test_suite_lib(editor_sculpt_paint "${TEST_SRC}" "${INC};${TEST_INC}" "${INC_SYS}" "${TEST_LIB}")
endif()
blender_add_lib(bf_editor_sculpt_paint "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")
# 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(WITH_TBB AND MSVC_CLANG)
string(APPEND CMAKE_CXX_FLAGS " /D_LIBCPP_VERSION")
endif()
blender_add_lib(bf_editor_sculpt_paint "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")
# RNA_prototypes.h
add_dependencies(bf_editor_sculpt_paint bf_rna)

View File

@ -288,6 +288,13 @@ list(APPEND LIB
bf_nodes_geometry_generated
)
# 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
Ben-7 marked this conversation as resolved Outdated

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
Outdated
Review

Added WITH_TBB check in 8556e4147f110.

Added `WITH_TBB` check in 8556e4147f110.
if(WITH_TBB AND MSVC_CLANG)
string(APPEND CMAKE_CXX_FLAGS " /D_LIBCPP_VERSION")
endif()
blender_add_lib(bf_nodes_geometry "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")
target_link_libraries(bf_nodes_geometry_generated bf_nodes_geometry)

View File

@ -1118,7 +1118,9 @@ elseif(WIN32)
)
Ben-7 marked this conversation as resolved Outdated

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?
Outdated
Review

No particular reason. I've removed them in edd2f667b9

No particular reason. I've removed them in edd2f667b961e02d106bdc67822fc51cc0603a6c
if(WITH_WINDOWS_RELEASE_PDB)
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)
# Icky hack for older CMAKE from https://stackoverflow.com/a/21198501
# `$<CONFIG>` will work in newer CMAKE but the version currently (3.12)
# on the build-bot does not support this endeavor.
@ -1851,7 +1853,8 @@ if(WIN32)
PDB_NAME "blender_private"
PDB_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>"
)
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)
# This is slightly messy, but single target generators like ninja will not have the
# `CMAKE_CFG_INTDIR` variable and multi-target generators like `msbuild` will not have
# `CMAKE_BUILD_TYPE`. This can be simplified by `target_link_options` and the `$<CONFIG>`