This is an archive of the discontinued LLVM Phabricator instance.

[OpenMPOpt][SplitMemTransfer] Implementation of the real issue and wait api functions.
Needs ReviewPublic

Authored by hamax97 on Sep 7 2020, 6:08 PM.

Details

Summary

Implementations of the issue and wait version of __tgt_target_data_begin_mapper.

Diff Detail

Event Timeline

hamax97 created this revision.Sep 7 2020, 6:08 PM
hamax97 requested review of this revision.Sep 7 2020, 6:08 PM
hamax97 updated this revision to Diff 293498.Sep 22 2020, 9:56 AM
hamax97 added a reviewer: jhuber6.
hamax97 set the repository for this revision to rG LLVM Github Monorepo.

Passing handle as input/output argument to __tgt_target_data_begin_mapper_<issue|wait>.

  • The diff ended up weird, I just added the declarations and definitions for __tgt_target_data_begin_mapper_issue and __tgt_target_data_begin_mapper_wait. I didn't deleted any code.
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2020, 9:56 AM

Ping!. Could you please take a look at it?

grokos added a subscriber: grokos.Sep 29 2020, 9:07 AM

I think these API functions should also include the source location pointer from https://reviews.llvm.org/D87946. We need to consider renaming the *_issue and *_wait functions to extend the *_loc API the aforementioned patch is introducing. E.g. after D87946 the "current" data begin API function will be __tgt_target_data_begin_mapper_loc, so this patch should extend that name as __tgt_target_data_begin_mapper_loc_issue and __tgt_target_data_begin_mapper_loc_wait. Because both patches make changes to the API, I think it's better to wait until the former patch has been committed.

I think these API functions should also include the source location pointer from https://reviews.llvm.org/D87946. We need to consider renaming the *_issue and *_wait functions to extend the *_loc API the aforementioned patch is introducing. E.g. after D87946 the "current" data begin API function will be __tgt_target_data_begin_mapper_loc, so this patch should extend that name as __tgt_target_data_begin_mapper_loc_issue and __tgt_target_data_begin_mapper_loc_wait. Because both patches make changes to the API, I think it's better to wait until the former patch has been committed.

We've also been talking about extending the API to optionally include declaration names in the mappers so we can refer to mapped variables by name in error and information messages. If we're breaking the API it's probably best to do it all at once so I'm not sure how soon we'll be able to push D87946.

I think these API functions should also include the source location pointer from https://reviews.llvm.org/D87946. We need to consider renaming the *_issue and *_wait functions to extend the *_loc API the aforementioned patch is introducing. E.g. after D87946 the "current" data begin API function will be __tgt_target_data_begin_mapper_loc, so this patch should extend that name as __tgt_target_data_begin_mapper_loc_issue and __tgt_target_data_begin_mapper_loc_wait. Because both patches make changes to the API, I think it's better to wait until the former patch has been committed.

Yes. It will be easier to add this patch after D87946 and not the other way around.