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.

Sign up for GitHub

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

Jump to bottom

Fix FindNVTX.cmake #3421

Merged
merged 4 commits into from Feb 28, 2022
Merged

Fix FindNVTX.cmake #3421

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
394e2a9
Fix FindNVTX.cmake
maxhgerlach Feb 25, 2022
45977ee
Set HINTS to find_path() rather than NVTX_ROOT
maxhgerlach Feb 25, 2022
74fd25f
Add CUDAToolkit_* HINTS in FindNCCL.cmake
maxhgerlach Feb 25, 2022
fc3ebe0
Fix lib paths for DDL
maxhgerlach Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmake/Modules/FindNVTX.cmake
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@

set(HOROVOD_NVTX_INCLUDE $ENV{HOROVOD_NVTX_INCLUDE} CACHE PATH "Folder containing NVIDIA NVTX3 headers")

list(APPEND NVTX_ROOT ${CUDAToolkit_LIBRARY_ROOT})
list(APPEND NVTX_ROOT ${CUDAToolkit_INCLUDE_DIRS})
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need to put ${CUDAToolkit_INCLUDE_DIRS} on line 19 as an extra hint to search include?
CMAKE_PREFIX_PATH takes the root dir and append bin, lib, or include for the appropriate search: https://cmake.org/cmake/help/latest/variable/CMAKE_PREFIX_PATH.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On my system, setting NVTX_ROOT properly seems to have already done the trick.

Would you say that adding this extra hint would help even more (on other systems)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For CMake 3.12+ (we require 3.13 now), setting <package>_ROOT correctly should be sufficient for find_path: https://cmake.org/cmake/help/latest/command/find_path.html

Copy link
Collaborator

@nvcastet nvcastet Feb 25, 2022 •
edited

Choose a reason for hiding this comment

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

It potentially ends up to be the same but CMAKE_PREFIX_PATH is usually used for the parent (root) directory instead of the subdir include. <PackageName>_ROOT is used in the same way. Specific subdir (bin/lib/include) hints are usually added to the corresponding find_* HINTS argument.
On my container CUDAToolkit_LIBRARY_ROOT is empty, which is weird, so relying on CUDAToolkit_INCLUDE_DIRS instead seems to be a good idea, but maybe it is set in some cases?
I would just have added CUDAToolkit_INCLUDE_DIRS to the find_path:

find_path(NVTX_INCLUDE_DIR
          NAMES nvtx3/nvToolsExt.h
          HINTS ${HOROVOD_NVTX_INCLUDE} ${CUDAToolkit_INCLUDE_DIRS})

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the end as you mentioned it works in both cases and here we seem to only need to search for just the include of nvtx. Out of curiosity, do you know why we don't need to find the nvtx lib too libnvToolsExt.so?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, understood. It would probably be clearer not to plug CUDAToolkit_INCLUDE_DIRS into NVTX_ROOT (although it works for us as we don't need the .so).

Out of curiosity, do you know why we don't need to find the nvtx lib too libnvToolsExt.so?

The Nsight Systems User Guide only speaks of the header file: https://docs.nvidia.com/nsight-systems/UserGuide/index.html#nvtx-trace Might it be necessary to explicitly link that library for use cases where one does not attach that profiler?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact we are using the nvtx header-only library, so the shared lib is never needed:
https://gitlab.kitware.com/cmake/cmake/-/issues/21377

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went with your suggestion, @nvcastet, there's no need to deal with the NVTX_ROOT variable at all here.

# Compatible layer for CMake <3.12:
list(APPEND CMAKE_PREFIX_PATH ${NVTX_ROOT})

Expand Down