[Openmp-commits] [PATCH] D70414: [libomptarget] Implement a CMakeLists.txt for amdgcn

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield marked 6 inline comments as done.
JonChesterfield added a comment.

This is actually quite close to compiling under Release, at least for cancel.cu and critical.cu which don't include omptarget-nvptx.h. Debug.h unconditionally includes support.h (looks unintentional) and amdgcn's target_impl doesn't work without the cuda shim.



================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:56
+  DIRECTORY)
+
+file(GLOB cuda_sources ${CMAKE_CURRENT_SOURCE_DIR}/src/*.cu
----------------
Hahnfeld wrote:
> JonChesterfield wrote:
> > Glob has the advantage that this file will keep working as other source is added to amdgcn or common.
> > 
> > This will break as soon as common contains a file that amdgcn doesn't want to compile. At that point, this can become a whitelist instead.
> > 
> > I'm happy to list out the sources explicitly if that's strongly preferred.
> As far as I know globs have the disadvantage that they need an explicit invocation of CMake to get an updated list of source files. That's why LLVM usually avoids them.
Fair enough. Explicit lists it is.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:70-71
+
+# create libraries
+set(mcpus $ENV{GFXLIST})
+
----------------
Hahnfeld wrote:
> I don't think it's a good idea to honor random environment variables. I'd suggest to make this a proper configuration variable.
Agreed. Added a variable loosely based on the equivalent one for nvptx and checked I can still plumb it into the out of tree build scripts.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:132
+  install(FILES ${OUTPUTDIR}/${bc_libname}
+     DESTINATION "lib${OPENMP_LIBDIR_SUFFIX}/libdevice"
+  )
----------------
Hahnfeld wrote:
> `OPENMP_INSTALL_LIBDIR`
Nice, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70414