[Openmp-commits] [PATCH] D111987: [libomptarget][nfc]Generalise DeviceRTL cmake to allow building for amdgpu

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:230
+  # require D112227 or similar to enable the compilation for amdgpu
+  # compileDeviceRTLLibrary(${mcpu} amdgpu -target amdgcn-amd-amdhsa "-D__AMDGCN__" -fvisibility=default -nogpulib)
 endforeach()
----------------
[nit] quotes not needed. I think neither for `__CUDA_ARCH__` but maybe it was intended to make clear that it is a single argument (in contrast of having forgotten as space before/after `${sm}`)


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:217
   # Copy library to destination.
+  # Note: This is acting on the llvm-link'ed library, not the opt'ed one
   add_custom_command(TARGET ${bclib_target_name} POST_BUILD
----------------
JonChesterfield wrote:
> Meinersbur wrote:
> > JonChesterfield wrote:
> > > Meinersbur wrote:
> > > > [not related to this patch] There seems to be a serious mistake:
> > > > ```
> > > >   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}_opt
> > > >       COMMAND ${OPT_TOOL} ${link_opt_flags} ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}
> > > >                       -o ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}
> > > > ```
> > > > Says it would generate the `${bclib_name}_opt` file, but the `-o` parameter points to `${bclib_name}`, overwriting the input file.
> > > Maybe? Install copies bclib_name, which was overwritten in place, so perhaps the OUTPUT statement doesn't need to match the file created.
> > > 
> > > It would certainly be clearer if we created ${bclib_name}_link.bc or similar as an intermediate file and fed that to opt to get ${bc_lib} which was then installed.
> > Commands should never overwrite in-place, it breaks the entire dependency system. 
> > 
> > To run two commands without anoter intermediate file, use two COMMAND in the same rule:
> > ```
> >   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}
> >       COMMAND ${LINK_TOOL} -o ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name} ${bc_files}
> >       COMMAND ${OPT_TOOL} ${link_opt_flags} ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name} -o ${CMAKE_CURRENT_BINARY_DIR}/${bclib_name}
> >       DEPENDS ${bc_files}
> >       COMMENT "Linking LLVM bitcode ${bclib_name}"
> >   )
> > ```
> this patch now has the halfway fix of patching up DEPENDS/OUTPUT for consistency, but doesn't attempt to merge the two custom commands together
Would have considered unrelated to this patch. Thanks for fixing anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111987