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

[CMake] add a switch for libm. #603

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

xuhancn
Copy link
Contributor

@xuhancn xuhancn commented Nov 13, 2024

Checklist

  • I have read the contributing guidelines.
  • I have considered portability of my change across platforms and architectures.
  • I have self-reviewed my code.
  • I have commented my code where necessary.
  • I have updated the documentation accordingly.
  • I have added tests that prove my fix is effective or that my feature works.

What is the purpose of this pull request?

  • Improve code quality or performance

What changes did you make?

Add a switch to turn off involve libm. It will bring duplicate math function on Windows.

  1. libm is special library, it is usually appeared on Linux, but we not use it on Windows.
  2. On Windows, Microsoft provided math functions on its runtime libraries, such as ucrt.lib.
  3. In some case, such as Intel compiler environment, Intel provided a libm for high performance purpose. Sleef's find_library(libm m) will involve the libm into project. So, we would occur the symbol conflict issue on math functions.

My solution is add a switch(option) to let upper CMake project turn off libm involvement. It default value is on, and keep compatible to original logical.

Does this PR relate to any existing issue?

Relates to/Fixes ([Intel GPU] Symbol conflict between libm.lib and ucrt.lib for Intel GPU on Windows)

Is there anything you would like reviewers to focus on?

Please on focus on...

@xuhancn xuhancn changed the title add a switch for libm. [CMake] add a switch for libm. Nov 13, 2024
@xuhancn
Copy link
Contributor Author

xuhancn commented Nov 13, 2024

@blapie could you please take a look for this PR?

@blapie
Copy link
Contributor

blapie commented Nov 15, 2024

Hi! Sorry I have not had the time this week, will look into next week.
The patch looks good to me. However it is not ideal to add yet another option, we need to figure out wether it is something that can be detected reliably instead.

@xuhancn
Copy link
Contributor Author

xuhancn commented Nov 22, 2024

Hi @blapie,

I known your concern, and for CMake detection, I have did the research.
PyTorch XPU build a little complex, there are two compilers on one build project.

  1. The MSVC(cl) as primary compiler for almost PyTorch C++ code build.
  2. Intel Compiler(icx) for XPU SYCL code build.

The detailed process as below:

  1. For XPU SYCL code build, we need source Intel Compiler to env vars. (It is like CUDA, but CUDA is not has libm)
  2. The torch_cpu.so should be build by MSVC, and its detection is for MSVC. At this time, find_library(libm m) will involved libm from Intel Compiler. (Intel implenmented libm for provide better math performance)
  3. MSVC is designed that, Windows runtime lib(ucrt.lib) provided math functions. The secondary libm will confuse it.

Add an additional option is simplest solution for this senario. And not impact on legacy release.

@blapie
Copy link
Contributor

blapie commented Dec 6, 2024

I'm going to merge the change but please be aware that adding an option is not the answer to all problems. ICC is not fully supported yet, so in the meantime this might be an acceptable workaround. As soon as ICC is supported we might switch to a more robust solution.

@blapie blapie merged commit 56e1f79 into shibatch:master Dec 6, 2024
36 checks passed
@xuhancn xuhancn deleted the add_switch_for_libm branch December 6, 2024 19:02
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Jan 11, 2025
This PR is implement of RFC: #141946
Changes:
1. Update `Sleef` to contains it's PRS: shibatch/sleef#603
2. Set `SLEEF_BUILD_WITH_LIBM` to `OFF`, it is turn off CMake find_library(libm) of `Sleef`.

Pull Request resolved: #142245
Approved by: https://github.com/EikanWang, https://github.com/atalman

Co-authored-by: Eikan Wang <eikan.wang@intel.com>
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.

[Intel GPU] Symbol conflict between libm.lib and ucrt.lib for Intel GPU on Windows
2 participants