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

Wed Mar 8 09:00:49 PST 2023

ye-luo accepted this revision.
ye-luo added inline comments.
This revision is now accepted and ready to land.

================
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:
> > > > 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.
Then what we have now is sufficient. Could you extend the error message saying current auto detection only support NV and AMD.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145513