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

[Michael Kruse via Phabricator via Openmp-commits]

Meinersbur added inline comments.


================
Comment at: openmp/libomptarget/DeviceRTL/CMakeLists.txt:145
 
-# Generate a Bitcode library for all the compute capabilities the user requested
-foreach(sm ${nvptx_sm_list})
-  # TODO: replace this with declare variant and isa selector.
-  set(cuda_flags -Xclang -target-cpu -Xclang sm_${sm} "-D__CUDA_ARCH__=${sm}0")
+macro(instantiate_DeviceRTL)
+  # parameters target_cpu, target_name, target_bc_flags
----------------
JonChesterfield wrote:
> Meinersbur wrote:
> > Consider
> > ```
> > function(instantiate_DeviceRTL target_cpu target_name)
> >   set(target_bc_flags ${ARGN})
> > ```
> > or maybe even `cmake_parse_arguments` of more options might be added in the future.
> I spent quite a while permuting function and macro trying to get names on parameters and none of it ran correctly, ending up with parameter names in a comment. 
> 
> Perhaps cmake thinks a function takes a list and peels off the named arguments, implicit &rest style?
> 
> How would one call such a function? 
> Perhaps cmake thinks a function takes a list and peels off the named arguments, implicit &rest style?

Yes. Like `*args` in Python. See https://cmake.org/cmake/help/latest/command/function.html#arguments

> How would one call such a function?
```
instantiate_DeviceRTL(sm_${SM} nvptx -target nvptx64 -Xclang -target-feature -Xclang +ptx61 "-D__CUDA_ARCH__=${sm}0")
```

With cmake_parse_arguments it could be something like
```
instantiate_DeviceRTL(
  TARGET nvptx
  ARCH sm_${SM} 
  TARGET_FLAGS -target nvptx64 -Xclang -target-feature -Xclang +ptx61 -D__CUDA_ARCH__=${sm}0
  )
```



================
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:
> > [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}"
  )
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111987