This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] [OMPT] [6/8] Added callback support for target data operations, target submit, and target regions.
ClosedPublic

Authored by mhalk on Jun 8 2022, 5:21 PM.

Details

Summary

This patch adds support for invoking target callbacks but does not yet
invoke them. A new structure OmptInterface has been added that tracks
thread local states including correlation ids. This structure defines
methods that will be called from the device independent target library
with information related to a target entry point for which a callback
is invoked. (The callback signature is defined by the OMPT spec and can be
found in omp-tools.h.) These methods in turn use the callback functions maintained
by OmptDeviceCallbacksTy to invoke the tool supplied callbacks.

Depends on D124652

Patch from John Mellor-Crummey <johnmc@rice.edu>
(With contributions from Dhruva Chakrabarti <Dhruva.Chakrabarti@amd.com>)

Diff Detail

Event Timeline

dhruvachak created this revision.Jun 8 2022, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 5:21 PM
dhruvachak requested review of this revision.Jun 8 2022, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 5:21 PM
dhruvachak retitled this revision from [OMPT] Added callback support for target data operations, target submit, and target regions. to [OpenMP] [OMPT] [6/8] Added callback support for target data operations, target submit, and target regions..Jun 20 2022, 10:54 PM
dhruvachak edited the summary of this revision. (Show Details)
jplehr added a subscriber: jplehr.Dec 1 2022, 8:04 AM
mhalk added a subscriber: mhalk.Feb 13 2023, 12:51 AM
mhalk updated this revision to Diff 499492.Feb 22 2023, 7:00 AM

Rebased + spelling fixes

mhalk updated this revision to Diff 506968.Mar 21 2023, 7:17 AM

Refactored patch for use with nextgen plugins.

mhalk commandeered this revision.Mar 21 2023, 8:37 AM
mhalk added a reviewer: dhruvachak.

As discussed with Dhruva.

Some initial comments

openmp/libomptarget/include/ompt_device_callbacks.h
101 ↗(On Diff #506968)

The methods here should probably read in an active voice, e.g., invokeOmptTargetDataOpEmiCallback and alike.

219 ↗(On Diff #506968)

No return needed.

openmp/libomptarget/src/OmptCallback.cpp
48

These functions should probably be updated to follow the LLVM naming convention, e.g., createId. Idk if this should be done right now, or in an NFC after the OMPT support has landed.

mhalk added inline comments.Mar 21 2023, 10:59 AM
openmp/libomptarget/src/OmptCallback.cpp
48

@NFC I was asking myself the same question.
But of course I could incorporate such NFCs in this set / stack of patches.

mhalk added inline comments.Mar 21 2023, 12:14 PM
openmp/libomptarget/include/ompt_device_callbacks.h
219 ↗(On Diff #506968)

Done.

parzt added a subscriber: parzt.Jul 5 2023, 8:12 AM
mhalk updated this revision to Diff 539060.Jul 11 2023, 6:27 AM

Update w.r.t. the landing of D124652; implement current state of feedback.

Added some nits.
Any more comments @jdoerfert @dreachem @dhruvachak before accepting this?

openmp/libomptarget/src/OmptCallback.cpp
66

This naming seems inconsistent, doesn't it?
Why is it called TargetDataValue in the OmptInterface but the function to access it getRegionId.

openmp/libomptarget/src/OmptInterface.h
44

This should probably be a class according to the coding standards (sorry if I'm only pointing to this now).
https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords

113

Parameter names here and below spelling starting w/ uppercase?

Thanks for pointing out these nits, I've prepped changes for the latter two (class & uppercase spelling) and will wait for further feedback.

openmp/libomptarget/src/OmptCallback.cpp
66

Yes, that is absolutely right.
Thanks for bringing this up.

Background:
According to the spec the underlying data type ompt_data_t is supposed to represent: "[...] data that is reserved for tool use and that is related to a thread or to a parallel or task region."

My observation so far is, that this is only used for (task / target) "regions".
From the use-case POV, maybe get[Target]RegionDataValue (get[Target]RegionDataPtr)?

But, "correctly", you'd call it getToolDataValue (getToolDataPtr) I guess -- while rather generic, that'd be my pick.

If anyone has a preference / other suggestion, I'm happy to oblige.

jplehr added inline comments.Jul 20 2023, 4:04 AM
openmp/libomptarget/src/OmptCallback.cpp
66

Ok, I re-read the spec. In the light of the spec the name regionId makes more sense.
Maybe a dumb question, but do we need this function, or is it better to return the ompt_data_t itself, so it is left to the caller what they want to do with it?
I mean, do we need / want / use this?

mhalk added inline comments.Jul 20 2023, 10:56 AM
openmp/libomptarget/src/OmptCallback.cpp
66

Now, at a closer look, after all the refactoring and moving we can simply remove getRegionId.
AFAIK and as discussed offline, getRegionId would have no other utility because of its visbility.

Its uses will be replaced with direct access to the corresponding TargetData.value within "non-EMI" callback functions.
The EMI callbacks already use the ompt_data_t TargetData field directly.

mhalk updated this revision to Diff 542609.Jul 20 2023, 11:36 AM

Rebased + removed getRegionId as it could be replaced.

jdoerfert accepted this revision.Jul 20 2023, 12:19 PM
This revision is now accepted and ready to land.Jul 20 2023, 12:19 PM