Page MenuHomePhabricator

[libomptarget][nfc]Generalise DeviceRTL cmake to allow building for amdgpu
ClosedPublic

Authored by JonChesterfield on Oct 18 2021, 4:52 AM.

Details

Summary

Essentially moves the foreach over sm integers into a macro and instantiates it for nvptx.

NFC in that the macro is not presently instantiated for amdgpu as the corresponding code doesn't compile yet.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Oct 18 2021, 4:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 4:52 AM
  • Disable amdgpu instantiation
JonChesterfield retitled this revision from [libomptarget][wip] Generalise DeviceRTL cmake to build for amdgpu to [libomptarget][wip]Generalise DeviceRTL cmake to allow building for amdgpu.Oct 18 2021, 7:11 AM
JonChesterfield edited the summary of this revision. (Show Details)

Can we split this patch to two, one is to generalize and another is to add AMD support?

This is the second half of that split, the refactor is in D111983.

Unfortunately I was unable to clearly express that in phabricator (worked initially, lost it in an update - if you check the history of this patch the first revision is what I'd have liked this to look like). So my hope is to land D111983 soon, which I don't think has any outstanding problems, and then rebase this to get something more legible.

JonChesterfield updated this revision to Diff 381231.EditedOct 21 2021, 5:48 AM
  • rebase try 2, nope - still showing relative to main
  • rebase, including D111983 via main

Rebased successfully now that refactor is on main, thanks Michael!

JonChesterfield retitled this revision from [libomptarget][wip]Generalise DeviceRTL cmake to allow building for amdgpu to [libomptarget][nfc]Generalise DeviceRTL cmake to allow building for amdgpu.Oct 21 2021, 8:18 AM
JonChesterfield edited the summary of this revision. (Show Details)
Meinersbur added inline comments.Oct 21 2021, 9:08 AM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
146–147

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.

215

[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.

openmp/libomptarget/DeviceRTL/CMakeLists.txt
146–147

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?

215

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.

Meinersbur added inline comments.Oct 22 2021, 10:52 AM
openmp/libomptarget/DeviceRTL/CMakeLists.txt
146–147

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
  )
215

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}"
)
  • macro->function, rework _opt suffix dependency hazard
  • drop outdated comment
openmp/libomptarget/DeviceRTL/CMakeLists.txt
146–147

Worked exactly as written in the comment, thanks!

215

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

Meinersbur accepted this revision.Oct 26 2021, 1:01 PM

LGTM

openmp/libomptarget/DeviceRTL/CMakeLists.txt
215

Would have considered unrelated to this patch. Thanks for fixing anyways.

230

[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})

This revision is now accepted and ready to land.Oct 26 2021, 1:01 PM
  • drop dead quotation marks

Thank you for the macro->function transform, much happier with the new form