[Openmp-commits] [PATCH] D88929: [OpenMP] Change CMake Configuration to Build for Highest CUDA Architecture by Default

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt:80
+      else()
+        list(APPEND compute_capabilities ${CMAKE_MATCH_1})
+      endif()
----------------
jhuber6 wrote:
> ye-luo wrote:
> > 1. Doesn't work right now. Missing comma ",${CMAKE_MATCH_1}"
> > 2. using CUDA_ARCH as "string(REGEX MATCH" output causes problems.
> > 3. "append" needs to protect redundant 35.
> > 4. I think it is better to move this part of logic to `openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake`
> > after `find_package(CUDA QUIET)`.
> 1. I noticed the comma problem, it's because the compute capabilities uses commas instead of semicolons like the rest of CMake to delimit the values. There's another weird error I'm getting while trying to build with this where it will add `sm_70` as an argument causing an nvcc error. like in `nvcc -o out file.cpp sm_70`.
> 2. What's the problem here? Pretty much the output gives some string that you would pass to nvcc like `--arch=sm_70` and I'm regexing out the `sm_70` if there was no match or the architecture is too small it doesn't add anything.
> 3. I'm thinking it would just be easiest to change it do something like `set(default_capabilities "35,${CMAKE_MATCH_1}")`
> 4. My feeling is that there's not enough complexity here to justify moving it since I'd need to add just as much code to generate the output and then even more to use it here. Since the LibomptargetGetDependencies.cmake doesn't bother checking whether or not the `find_package(CUDA)` was successful I feel like there's no need to add logic that requires checking if it was there when we already have it here.
3. better than append but still need a check to avoid "35,35"
4. https://github.com/ye-luo/llvm-project/commit/ac5f20f9770e894ff48467a9317ec0649f5c7562
libomptarget part should be fulling working.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88929/new/

https://reviews.llvm.org/D88929