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

add sleef prefix to cmake options. #509

Merged
merged 5 commits into from Feb 7, 2024

Conversation

xuhancn
Copy link
Contributor

@xuhancn xuhancn commented Feb 4, 2024

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 xuhancn marked this pull request as ready for review February 4, 2024 13:34
@xuhancn
Copy link
Contributor Author

xuhancn commented Feb 4, 2024

@blapie Please review this PR. Thanks.

@xuhancn
Copy link
Contributor Author

xuhancn commented Feb 4, 2024

image Local test.

@blapie
Copy link
Collaborator

blapie commented Feb 5, 2024

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

xuhancn commented Feb 5, 2024

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
Copy link
Collaborator

blapie commented Feb 5, 2024

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

xuhancn commented Feb 5, 2024

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
Copy link
Collaborator

blapie commented Feb 6, 2024

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)

CMakeLists.txt Outdated
option(BUILD_SCALAR_LIB "libsleefscalar will be built." OFF)
option(BUILD_TESTS "Tests will be built." ON)
option(BUILD_INLINE_HEADERS "Build header for inlining whole SLEEF functions" OFF)
option(SLEEF_BUILD_SHARED_LIBS "Build shared libs" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We think this option should be removed altogether and we should keep using the standard cmake variable BUILD_SHARED_LIBS in the rest of the build system. This way we ensure consistency with projects using SLEEF.
Remember that even if turned off libsleefgnuabi will be a shared library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not set SLEEF_BUILD_SHARED_LIBS , and then use BUILD_SHARED_LIBS ? let SLEEF_BUILD_SHARED_LIBS overwrite BUILD_SHARED_LIBS?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename all SLEEF_BUILD_SHARED_LIBS to BUILD_SHARED_LIBS, then remove this line

option(BUILD_SHARED_LIBS "Build shared libs" ON)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Please rename all SLEEF_BUILD_SHARED_LIBS to BUILD_SHARED_LIBS, then remove this line

option(BUILD_SHARED_LIBS "Build shared libs" ON)

# Disable VXE2 support, QEMU doesn't support some instructions generated by gcc or llvm
EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} -DDISABLE_VXE2=ON"
EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} -DSLEEF_DISABLE_VXE2=ON"
elif [[ ${{ matrix.arch }} = "riscv64" ]]; then
EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} -DENFORCE_RVVM1=ON -DENFORCE_RVVM2=ON"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These also would need prefixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@blapie
Copy link
Collaborator

blapie commented Feb 7, 2024

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

@blapie blapie merged commit bd7ae3a into shibatch:master Feb 7, 2024
31 checks passed
@xuhancn xuhancn mentioned this pull request Feb 8, 2024
@xuhancn xuhancn deleted the xu_optimize_build_options branch March 19, 2024 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants