This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] [OMPT] [7/8] Invoke tool-supplied callbacks before and after target launch and data transfer operations
ClosedPublic

Authored by mhalk on Jun 8 2022, 6:01 PM.

Details

Summary

Implemented RAII objects, initialized at target entry points, that
invoke tool-supplied callbacks. Updated status of target callbacks as
implemented.

Depends on D127365

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, 6:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 6:01 PM
dhruvachak requested review of this revision.Jun 8 2022, 6:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 6:01 PM
dhruvachak retitled this revision from [OMPT] Invoke tool-supplied callbacks before and after target launch and data transfer operations to [OpenMP] [OMPT] [7/8] Invoke tool-supplied callbacks before and after target launch and data transfer operations.Jun 20 2022, 10:58 PM
jplehr added a subscriber: jplehr.Dec 1 2022, 8:04 AM
jplehr added inline comments.Dec 1 2022, 1:02 PM
openmp/libomptarget/src/device.cpp
594

I wonder if these RAII objects are constructed when OMPT support is not built, or if it is optimized away in such cases?

624

In the asynchronous case, the lifetime of the RAII object is probably too short to account for the actual data transfer events.

651

In the asynchronous case, the lifetime of the RAII object is probably too short to account for the actual data transfer events.

mhalk added a subscriber: mhalk.Feb 13 2023, 12:50 AM
mhalk updated this revision to Diff 499493.Feb 22 2023, 7:02 AM

Rebased

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

Refactored patch for use with nextgen plugins.

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

As discussed with Dhruva.

jdoerfert added inline comments.Mar 22 2023, 5:45 PM
openmp/libomptarget/src/device.cpp
42

Style, also names of the arguments.

openmp/libomptarget/src/interface.cpp
82

These RAIIs should stay together. If you make them templated you could even get rid of the individual members and potentially have a single RAII to rule them all. (you'd pass the callback rather than the enum, or have a separate switch for the enum to callback translation, either way, you'd then unpack the tuple for the call, see std::apply)

openmp/libomptarget/src/omptarget.cpp
35

if you move the conditional into the con/destructor and you create a common base class for these, you should be able to eliminate all but the logic dealing with the provided values (here NumTeams).

1571

*, not +

mhalk updated this revision to Diff 542612.Jul 20 2023, 11:38 AM

Rebase + implemented feedback so far. Thanks @jdoerfert!
Exception: RAII objects have not been consolidated, yet.

mhalk added inline comments.Jul 20 2023, 12:06 PM
openmp/libomptarget/src/OmptCallback.cpp
45 ↗(On Diff #542612)

Some sort of init flag will be needed.
Originally thought the timeline would allow to go with a separate patch.
See D155186

openmp/libomptarget/src/device.cpp
46

Oversight: Six occurrences -- will be removed.

jdoerfert added inline comments.Jul 20 2023, 12:22 PM
openmp/libomptarget/src/OmptCallback.cpp
39 ↗(On Diff #542612)

Why is the comment gone?

openmp/libomptarget/src/omptarget.cpp
40

Commented code?

mhalk added inline comments.Jul 20 2023, 12:30 PM
openmp/libomptarget/src/OmptCallback.cpp
39 ↗(On Diff #542612)

Has been moved to its declaration in openmp/libomptarget/src/OmptInterface.h.

openmp/libomptarget/src/omptarget.cpp
40

Yes, my bad -- as stated in my other comment there are six occurrences, all of which will be removed upon the next change.
performIfOmptInitialized already checks the Variable before calling [begin|end]Submit().

mhalk added inline comments.Jul 20 2023, 12:47 PM
openmp/libomptarget/src/omptarget.cpp
40

FYI: I' will adapt this "part" of the patch (using an Init flag) upon the landing of D155186.

mhalk updated this revision to Diff 543565.Jul 24 2023, 8:18 AM

Implemented templated RAII class.

Please, take a look if this is the desired direction.
If the approach should be changed, let me know.

Other than that, there are a few outstanding issues:
(Currently looking into the first two.)

  • Warnings are currently emitted for each RAII instance
warning: 'InterfaceRAII' may not intend to support class template argument
         deduction [-Wctad-maybe-unsupported]
note: add a deduction guide to suppress this warning
  • Test coverage should be expanded for target region operations: enter_data, exit_data, update
    • Those cases are handled by the RAII -- but simply not tested.
    • Will add another lit test in another upcoming review.
  • Asynchronous timing does not work correctly as stated in a previous comment. RAII objects will be prematurely destroyed in this use case. Please, let me know if this is a requirement for accepting this patch.
jdoerfert accepted this revision.Jul 24 2023, 9:51 AM

Add -Wno-ctad-maybe-unsupported to our cmake config or add sth like:

template <typename FunctionPairTy, typename... ArgsTy>
  InterfaceRAII(FunctionPairTy FunctionPair, ArgsTy... Args) -> InterfaceRAII< FunctionPairTy, ArgsTy...>;

We can fix async in a follow up.

This revision is now accepted and ready to land.Jul 24 2023, 9:51 AM
dhruvachak added inline comments.Jul 24 2023, 10:09 AM
openmp/libomptarget/src/device.cpp
624

I think the asynchronous case is not an issue for the callbacks. The begin and end callbacks are supposed to be invoked before and after the entry-point invocation. The actual data transfer may complete later and is supposed to be captured by device tracing, something that is not part of this patch.

mhalk updated this revision to Diff 543675.Jul 24 2023, 12:37 PM

Fixed warning related to template deduction -- thanks @jdoerfert
Fixed constant numbers being used in target data operations
NFC / improved naming of InterfaceRAII

Known issue:
Missing test coverage for target region operations: enter_data, exit_data, update
Callback my fire before the actual device init and report wrong device number

  • Saw this with enter_data when performing a datamapping within a new testcase

Currently, I plan on landing this within the next five hours.
If there are concerns or further feedback, let me know.

*ping* @jdoerfert @dhruvachak

I don't think the code in common path in this patch has been guarded correctly. It causes build error if OMPT is disabled. Please fix it or get it reverted if it takes long time.

mhalk added a comment.Jul 25 2023, 3:23 AM

Thanks @tianshilei1992 for bringing this to my attention (again)!
Sorry for the inconvenience, I will try to fix this ASAP.

mhalk reopened this revision.Jul 25 2023, 4:25 AM
This revision is now accepted and ready to land.Jul 25 2023, 4:25 AM
mhalk updated this revision to Diff 543922.Jul 25 2023, 4:25 AM

Fixed build errors when OMPT support was disabled.

jplehr accepted this revision.Jul 25 2023, 5:06 AM

LGTM, assuming build works

openmp/libomptarget/src/omptarget.cpp
1573

nit: Why?

mhalk added inline comments.Jul 25 2023, 5:19 AM
openmp/libomptarget/src/omptarget.cpp
1573

Because we're already checking via #ifdef OMPT_SUPPORT