[Openmp-commits] [PATCH] D145513: [Libomptarget] Update handling of architectures for DeviceRTL

Wed Mar 8 08:47:24 PST 2023

jhuber6 added inline comments.

================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:74
+elseif(LIBOMPTARGET_DEVICE_ARCHITECTURES STREQUAL "auto")
+  if (NOT LIBOMPTARGET_NVPTX_ARCH AND NOT LIBOMPTARGET_AMDGPU_ARCH)
+    libomptarget_error_say(
----------------
ye-luo wrote:
> jhuber6 wrote:
> > ye-luo wrote:
> > > Are both tool always built together?
> > Yes, they are always built by `clang`.
> Did you mean they are always built by the clang target outside openmp project?
> If I don't build NVPTX or AMDGPU backend, are they still being built?
> Can it be better if we do this.
> if nvptx-arch exists, add its list to the LIBOMPTARGET_DEVICE_ARCHITECTURES.
> if it doesn't exists, print a warning but don't error out.
> do the same for amdgpu-arch.
> if LIBOMPTARGET_DEVICE_ARCHITECTURES is still empty, error out.
`nvptx-arch` and `amdgpu-arch` are clang tools. They are built unconditionally whenever you build `clang`, they will be available here during a runtimes build, or if the user has the binary somewhere else. This is only necessary for `auto` support, which I'm pretty sure already used this transitively after I made a previous change deprecating the use of `findCUDA` in favor of `nvptx-arch`.

This logic only runs on `auto` which I feel is fair to require some additional handling to support. Or are you suggesting we make the default only supporting the user's architecture?

================
Comment at: openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake:121
     set(LIBOMPTARGET_FOUND_NVIDIA_GPU TRUE)
-    set(LIBOMPTARGET_DEP_CUDA_ARCH "${arch_string}")
+    set(LIBOMPTARGET_NVPTX_ARCH_LIST "${nvptx_arch_list}")
+    list(GET nvptx_arch_list 0 LIBOMPTARGET_DEP_CUDA_ARCH)
----------------
ye-luo wrote:
> There are a few variables related to ARCH. Could you rename this to LIBOMPTARGET_NVPTX_ARCH_DETECTED_LIST.
> Also the AMDGPU one.
Sure

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145513