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

Build system upgrades #281

Merged
merged 2 commits into from
Jan 7, 2019
Merged

Build system upgrades #281

merged 2 commits into from
Jan 7, 2019

Conversation

snnn
Copy link
Member

@snnn snnn commented Jan 5, 2019

  1. Add a new cmake option:onnxruntime_BUILD_FOR_NATIVE_MACHINE. This should be enabled before doing any perf test on Linux.
  2. Delete onnxruntime_CUDA_HOME cmake option. CUDA path and version will be controlled by standard cmake options.
  3. Make onnxruntime_USE_OPENMP option switchable on Linux
  4. Remove "find_package(CUDA)". We already have "enable_language(CUDA)". We only need one of them
  5. fix some warnings in op kernels.
  6. Move ONNX schema initialization from lazy-init to InitONNXEnv
  7. Add --skip_submodule_sync to windows CI build pipelines
  8. Upgrade cmake to 3.13. The next time, if anyone want to upgrade cmake, please change tools/ci_build/github/linux/docker/scripts/install_deps.sh and tools/ci_build/github/windows/download_cmake.py
  9. Disable gcc's null pointer checker. Too many warnings
  10. Run the linux build as a non-root user.

More words on the CUDA part:
There are two ways of enabling CUDA in cmake:

  1. "find_package(CUDA)", deprecated since cmake 3.10
  2. "enable_language(CUDA)", available from cmake 3.8

And, old cmake can't work with newer CUDA version very well. It's reasonable. It's like you can't expect a compiler release in 2014 has well support for c++17. CUDA is changing, therefore we should get our cmake up to date. And we will not try to support older cmake versions. Installing cmake is relatively easy. Based on that, for the two options in front of me, I would choose the newer one.

However, the second one introduced a few problems. The biggest is, if you have code like:

string(APPEND CMAKE_CXX_FLAGS "-pthread")

You must change it to:

string(APPEND CMAKE_CXX_FLAGS "$<$<COMPILE_LANGUAGE:CUDA>:SHELL:-Xcompiler -pthread>"
            "$<$<NOT:$<COMPILE_LANGUAGE:CUDA>>:-pthread>"

And because of that, the most existing cmake modules are broken. If the code was written before cmake 3.8 was release, it's unlikely it can continue to work. Notably, FindThreads, googletest.

Let me talk them one by one.
On Linux, there are two compiler flags for enabling multiple threading. You can choose either of them.

  1. '-pthread', could be apply to both compiling and linking
  2. '-lpthread', linking only.
    If I make the story shorter, the first one is deprecated, at least in the last 20 years, glibc doesn't need it. The second one is totaly enough. CMake FindThreads module has an option for choosing among them, but, its behavior is very strange and it doesn't always work.
    If the cmake variable "CMAKE_THREAD_LIBS_INIT" is evaluated to '-pthread', it will break the CUDA build. Because nvcc can't understand this arg. However, googletest explicitly says he prefer '-pthread' to '-lpthread'. Nobody was wrong, expect cmake itself. The FindThreads module is updated in cmake 3.13 and this problem is fixed.

@snnn snnn requested a review from a team as a code owner January 5, 2019 04:22
@raymondxyang raymondxyang merged commit 5e11366 into master Jan 7, 2019
@raymondxyang raymondxyang deleted the snnn/cmake branch January 7, 2019 21:15
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.

2 participants