Page MenuHomePhabricator

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

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

Details

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

Repository
rL LLVM

Event Timeline

jdenny created this revision.Dec 11 2018, 5:10 PM
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.

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 ↗(On Diff #178681)

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 ↗(On Diff #178681)

LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR

jdenny added inline comments.Dec 19 2018, 3:27 PM
openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116 ↗(On Diff #178681)

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 ↗(On Diff #178681)

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.

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

LGTM.

openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116 ↗(On Diff #178681)

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 ↗(On Diff #178681)

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 ↗(On Diff #178681)

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 ↗(On Diff #178681)

Yes, that's pretty much it :)

This revision was automatically updated to reflect the committed changes.
jdenny marked 8 inline comments as done.Jan 3 2019, 6:23 PM
jdenny added inline comments.
openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake
116 ↗(On Diff #178681)

Thanks, and thanks for the review!