This is an archive of the discontinued LLVM Phabricator instance.

[NFC][OpenMP] Prepare amdgpu plugin for asynchronous implementation of target region launch
ClosedPublic

Authored by carlo.bertolli on Dec 7 2021, 10:40 AM.

Details

Summary

At present, amdgpu plugin merges both asynchronous and synchronous kernel launch implementations into a single synchronous version.
This patch prepares the plugin for asynchronous implementation by:

  • Privatizing actual kernel launch code (valid in both cases) into an anonymous namespace base function

Actual separation of kernel launch code (async vs sync) is a following patch.

Diff Detail

Event Timeline

carlo.bertolli created this revision.Dec 7 2021, 10:40 AM
carlo.bertolli requested review of this revision.Dec 7 2021, 10:40 AM
JonChesterfield added a comment.EditedDec 7 2021, 12:16 PM

This patch is almost pure code motion. Rearranging in a diff tool the non-motion part is deletion of:

static int32_t __tgt_rtl_run_target_team_region_locked(
    int32_t device_id, void *tgt_entry_ptr, void **tgt_args,
    ptrdiff_t *tgt_offsets, int32_t arg_num, int32_t num_teams,
    int32_t thread_limit, uint64_t loop_tripcount);

which means we've lost the static annotation. Aside from that, LG

openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
1046–1052

This was previously static, please reintroduce that annotation.

carlo.bertolli added inline comments.Dec 7 2021, 12:48 PM
openmp/libomptarget/plugins/amdgpu/src/rtl.cpp
1046–1052

It is now located in an anonymous namespace which makes static redundant. If you want it, I can put it back, but semantically there is no difference.

JonChesterfield accepted this revision.Dec 7 2021, 12:56 PM

Oh, cool - missed the namespace{} as it wasn't in the diff. All good then, the change in name mangling from static -> anonymous doesn't matter.

This revision is now accepted and ready to land.Dec 7 2021, 12:56 PM

@JonChesterfield thanks for the quick review. Would you mind merging this for me?

Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 1:03 PM