This is an archive of the discontinued LLVM Phabricator instance.

Differential D55588

[OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu
ClosedPublic

Authored by jdenny on Dec 11 2018, 5:10 PM.

Details

Reviewers
ABataev
Hahnfeld
tra
sylvestre.ledru
grokos
AndreyChurbanov
Commits
rGf17f7a5d4d36: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu
rOMP350377: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu
rL350377: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu
Summary

The OpenMP runtime's cmake scripts do not correctly locate the
libdevice that the Debian/Ubuntu package nvidia-cuda-toolkit currently
includes, at least on my Ubuntu 18.04.1 installation. This patch
fixes that for me.

This problem was discussed at length in D55269. D40453 added a
similar adjustment in clang, but reviewers of D55269 concluded that,
for the OpenMP runtime, the right place to address this problem is in
cmake's CUDA support. However, it was also suggested we could add a
workaround to OpenMP's cmake scripts now. This patch contains such a
workaround, which I've tried to design so that it will have no harmful
effect if cmake improves in the future.

nvidia-cuda-toolkit also needs improvements because its intended
monolithic CUDA tree shim, /usr/lib/cuda, has many empty directories,
such as bin. I reported that at:

https://bugs.launchpad.net/ubuntu/+source/nvidia-cuda-toolkit/+bug/1808999

Diff Detail

Event Timeline

jdenny created this revision.Dec 11 2018, 5:10 PM
Herald added subscribers: guansong, kristof.beyls, javed.absar, mgorny. · View Herald TranscriptDec 11 2018, 5:11 PM
jdenny added a reviewer: AndreyChurbanov.Dec 14 2018, 3:00 PM
Herald added a subscriber: michaelplatings. · View Herald TranscriptDec 14 2018, 3:00 PM
michaelplatings removed a subscriber: michaelplatings.Dec 17 2018, 1:12 AM
jdenny updated this revision to Diff 178681.Dec 18 2018, 8:07 AM
jdenny edited the summary of this revision. (Show Details)
  • Fix a comment typo.
  • Update summary with nvidia-cuda-toolkit bug report I just filed.
grokos added a comment.Dec 18 2018, 7:16 PM

Since a similar solution was adopted for clang, I think we should let this one land. After all, it's a matter of consistency between the two projects.

openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116

There's a convention that cmake variables are capitalized. Also, cmake variables local to libomptarget should start with the prefix LIBOMPTARGET_. So this variable's name should be changed to LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET ("preset" makes more sense than "old").

172

LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR

jdenny added inline comments.Dec 19 2018, 3:27 PM
openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116

Thanks for the review. Sure, I'll make those changes.

However, I had thought that non-cached or local variables are usually lowercase. Is there a distinction here I'm not seeing, or is that just a different style than you prefer here?

185

Should candidate_dir be LIBOMPTARGET_CANDIDATE_DIR?

jdenny updated this revision to Diff 179083.Dec 20 2018, 7:56 AM
jdenny edited the summary of this revision. (Show Details)
jdenny set the repository for this revision to rOMP OpenMP.

Apply variable renames suggested by grokos.

jdenny added a comment.Jan 2 2019, 2:09 PM

Ping?

grokos accepted this revision.Jan 3 2019, 5:11 AM

LGTM.

openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116

True, local variables are lowercase, but non-cached variables are still capitalized. For LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET and LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR I prefer the capitalized version because these variables may be used outside this file in the future (although right now they are only used locally).

185

It's OK, it's only local.

This revision is now accepted and ready to land.Jan 3 2019, 5:11 AM
jdenny marked an inline comment as done.Jan 3 2019, 4:06 PM
jdenny added inline comments.
openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116

So your decision to capitalize in this case is based on your expectations about how usage could evolve in the future. Conversely, the candidate_dir from the other comment could be considered non-local, but your expectation is that it won't ever be used that way. Does all that sound right? That's all fine. I just want to be sure I understand. Thanks.

grokos added inline comments.Jan 3 2019, 5:43 PM
openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116

Yes, that's pretty much it :)

Closed by commit rL350377: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu (authored by jdenny). · Explain WhyJan 3 2019, 6:10 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: llvm-commits. · View Herald TranscriptJan 3 2019, 6:11 PM
jdenny marked 8 inline comments as done.Jan 3 2019, 6:23 PM
jdenny added inline comments.
openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116

Thanks, and thanks for the review!

jdenny mentioned this in D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu.Mar 14 2019, 8:27 AM
Meinersbur mentioned this in D89974: [driver][CUDA] Use CMake's FindCUDA as default --cuda-path..Oct 22 2020, 10:27 AM

Revision Contents

PathSize
openmp/
libomptarget/
cmake/
Modules/
32 lines

Diff 177801

openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake

Show First 20 Lines • Show All 106 Lines • ▼ Show 20 Lines
mark_as_advanced( mark_as_advanced(
LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIRS LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIRS
LIBOMPTARGET_DEP_LIBFFI_LIBRARIES) LIBOMPTARGET_DEP_LIBFFI_LIBRARIES)
################################################################################ ################################################################################
# Looking for CUDA... # Looking for CUDA...
################################################################################ ################################################################################
if (CUDA_TOOLKIT_ROOT_DIR)
set(cuda_toolkit_root_dir_old TRUE)
grokosUnsubmitted
Done
ReplyInline Actions

There's a convention that cmake variables are capitalized. Also, cmake variables local to libomptarget should start with the prefix LIBOMPTARGET_. So this variable's name should be changed to LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET ("preset" makes more sense than "old").

grokos: There's a convention that cmake variables are capitalized. Also, cmake variables local to…
jdennyAuthorUnsubmitted
Done
ReplyInline Actions

Thanks for the review. Sure, I'll make those changes.

However, I had thought that non-cached or local variables are usually lowercase. Is there a distinction here I'm not seeing, or is that just a different style than you prefer here?

jdenny: Thanks for the review. Sure, I'll make those changes. However, I had thought that non-cached…
grokosUnsubmitted
Done
ReplyInline Actions

True, local variables are lowercase, but non-cached variables are still capitalized. For LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET and LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR I prefer the capitalized version because these variables may be used outside this file in the future (although right now they are only used locally).

grokos: True, local variables are lowercase, but non-cached variables are still capitalized. For…
jdennyAuthorUnsubmitted
Done
ReplyInline Actions

So your decision to capitalize in this case is based on your expectations about how usage could evolve in the future. Conversely, the candidate_dir from the other comment could be considered non-local, but your expectation is that it won't ever be used that way. Does all that sound right? That's all fine. I just want to be sure I understand. Thanks.

jdenny: So your decision to capitalize in this case is based on your expectations about how usage could…
grokosUnsubmitted
Not Done
ReplyInline Actions

Yes, that's pretty much it :)

grokos: Yes, that's pretty much it :)
jdennyAuthorUnsubmitted
Done
ReplyInline Actions

Thanks, and thanks for the review!

jdenny: Thanks, and thanks for the review!
endif()
find_package(CUDA QUIET) find_package(CUDA QUIET)
set(LIBOMPTARGET_DEP_CUDA_FOUND ${CUDA_FOUND}) set(LIBOMPTARGET_DEP_CUDA_FOUND ${CUDA_FOUND})
set(LIBOMPTARGET_DEP_CUDA_INCLUDE_DIRS ${CUDA_INCLUDE_DIRS}) set(LIBOMPTARGET_DEP_CUDA_INCLUDE_DIRS ${CUDA_INCLUDE_DIRS})
mark_as_advanced( mark_as_advanced(
LIBOMPTARGET_DEP_CUDA_FOUND LIBOMPTARGET_DEP_CUDA_FOUND
LIBOMPTARGET_DEP_CUDA_INCLUDE_DIRS) LIBOMPTARGET_DEP_CUDA_INCLUDE_DIRS)
Show All 30 Lines
endif() endif()
find_package_handle_standard_args( find_package_handle_standard_args(
LIBOMPTARGET_DEP_CUDA_DRIVER LIBOMPTARGET_DEP_CUDA_DRIVER
DEFAULT_MSG DEFAULT_MSG
LIBOMPTARGET_DEP_CUDA_DRIVER_LIBRARIES) LIBOMPTARGET_DEP_CUDA_DRIVER_LIBRARIES)
mark_as_advanced(LIBOMPTARGET_DEP_CUDA_DRIVER_LIBRARIES) mark_as_advanced(LIBOMPTARGET_DEP_CUDA_DRIVER_LIBRARIES)
################################################################################
# Looking for CUDA libdevice subdirectory
#
# Special case for Debian/Ubuntu to have nvidia-cuda-toolkit work
# out of the box. More info on http://bugs.debian.org/882505
################################################################################
set(cuda_libdevice_subdir nvvm/libdevice)
grokosUnsubmitted
Done
ReplyInline Actions

LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR

grokos: `LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR`
# Don't alter CUDA_TOOLKIT_ROOT_DIR if the user specified it, if a value was
# already cached for it, or if it already has libdevice Otherwise, on
# Debian/Ubuntu, look where the nvidia-cuda-toolkit package normally installs
# libdevice.
if (NOT cuda_toolkit_root_dir_old AND
NOT EXISTS "${CUDA_TOOLKIT_ROOT_DIR}/${cuda_libdevice_subdir}")
find_program(LSB_RELEASE lsb_release)
if (LSB_RELEASE)
execute_process(COMMAND ${LSB_RELEASE} -is
OUTPUT_VARIABLE LSB_RELEASE_ID
OUTPUT_STRIP_TRAILING_WHITESPACE)
set(candidate_dir /usr/lib/cuda)
jdennyAuthorUnsubmitted
Done
ReplyInline Actions

Should candidate_dir be LIBOMPTARGET_CANDIDATE_DIR?

jdenny: Should candidate_dir be LIBOMPTARGET_CANDIDATE_DIR?
grokosUnsubmitted
Done
ReplyInline Actions

It's OK, it's only local.

grokos: It's OK, it's only local.
if ((LSB_RELEASE_ID STREQUAL "Debian" OR LSB_RELEASE_ID STREQUAL "Ubuntu")
AND EXISTS "${candidate_dir}/${cuda_libdevice_subdir}")
set(CUDA_TOOLKIT_ROOT_DIR "${candidate_dir}" CACHE PATH
"Toolkit location." FORCE)
endif()
endif()
endif()