shibatch / sleef

SIMD Library for Evaluating Elementary Functions, vectorized libm and DFT
https://sleef.org
Boost Software License 1.0
628 stars 126 forks source link

add sleef prefix to cmake options. #509

Closed xuhancn closed 6 months ago

xuhancn commented 6 months ago

I'm work on enable sleef on pytorch Windows version, and have some build option issue when second build. Error as below:

image

I read the pytorch cmake file and found that, sleef build options "BUILD_SHARED_LIBS" and "BUILD_TESTS" was conflict to pytorch. Current pytorch save and restore build options to cmake cache and workarround it. Save: https://github.com/pytorch/pytorch/blob/cee16353db384dd33a4804f9f26829cacfcfbab7/aten/src/ATen/CMakeLists.txt#L424-L440 Restore: https://github.com/pytorch/pytorch/blob/cee16353db384dd33a4804f9f26829cacfcfbab7/aten/src/ATen/CMakeLists.txt#L480-L485 When secound build the cache will have unexpected behavior and lost some build options, and then different build option will occurred issues.

A good example is MSFT mimalloc. Its build options has its prefix and will not conflict to others: https://github.com/microsoft/mimalloc/blob/master/CMakeLists.txt#L7-L34 Its caller like this: https://github.com/pytorch/pytorch/blob/cee16353db384dd33a4804f9f26829cacfcfbab7/CMakeLists.txt#L1061-L1067

I have tried to add sleef prefix to its cmake files. After this PR, It will help me to enable sleef on pytorch Windows.

xuhancn commented 6 months ago

@blapie Please review this PR. Thanks.

xuhancn commented 6 months ago
image

Local test.

blapie commented 6 months ago

Hi! Good initiative. Is there a reason for not putting SLEEF_ as a prefix in options starting with BUILD_? This is a relatively significant change for users, therefore I cannot guarantee this is going to be part of the 3.6 release, unless there is a good reason for it, I'll let you know.

xuhancn commented 6 months ago

Hi! Good initiative. Is there a reason for not putting SLEEF_ as a prefix in options starting with BUILD_? This is a relatively significant change for users, therefore I cannot guarantee this is going to be part of the 3.6 release, unless there is a good reason for it, I'll let you know.

Updated all build options, and let then start with "SLEEF_".

blapie commented 6 months ago

Would be good to do the same for the rest of the cmake variables. There are a bunch in docs/build-with-cmake.md and most of them are defined in Configure.cmake.

xuhancn commented 6 months ago

Would be good to do the same for the rest of the cmake variables. There are a bunch in docs/build-with-cmake.md and most of them are defined in Configure.cmake.

Done.

blapie commented 6 months ago

Just a remark. BUILD_SHARED_LIBS is a standard cmake variable. It seems that SLEEF did overwrite it, which is not recommended. Renaming sounds like a good idea since the effect of this option seem to differ slightly from the standard, and implement its own custom rules (#132). Namely if SLEEF_BUILD_SHARED_LIBS is ON (default) then we generate shared libraries for all targets, otherwise we don't except for libsleefgnuabi that is always a shared library. However, if we rename the variable then both options (SLEEF_BUILD_SHARED_LIBS and BUILD_SHARED_LIBS) might need to match. The other option is to simply delete the redefinition of BUILD_SHARED_LIBS in Sleef and keep the standard name.

option(BUILD_SHARED_LIBS "Build shared libs" ON)
blapie commented 6 months ago

Thank you! This is looking good, will merge. Thank you for your contribution.