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

Wed Mar 8 08:53:18 PST 2023

ye-luo 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(
----------------
jhuber6 wrote:
> 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?
What I described above is intended for inside the logic block "auto". I don't support having auto as the default.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145513