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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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
Collaborator

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.

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