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

Wed Mar 8 08:54:17 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:
> > > 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.
I think this is fine since there's no situation where the user would have one but not the other unless they manually deleted it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145513