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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Random drive-by comments to parts of the patch, I won't review in full.



================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:56
+  DIRECTORY)
+
+file(GLOB cuda_sources ${CMAKE_CURRENT_SOURCE_DIR}/src/*.cu
----------------
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.


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:70-71
+
+# create libraries
+set(mcpus $ENV{GFXLIST})
+
----------------
I don't think it's a good idea to honor random environment variables. I'd suggest to make this a proper configuration variable.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70414