This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] [OMPT] [amdgpu] [5/8] Implemented device init/fini/load callbacks
ClosedPublic

Authored by mhalk on Apr 28 2022, 6:44 PM.

Details

Summary

Added support in the generic plugin to invoke registered callbacks.

Depends on D124070

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.Apr 28 2022, 6:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 6:44 PM
Herald added subscribers: kerbowa, t-tye, tpr and 4 others. · View Herald Transcript
dhruvachak requested review of this revision.Apr 28 2022, 6:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 6:44 PM
dhruvachak retitled this revision from [OMPT] [amdgpu] Implemented device init/fini/load callbacks to [OpenMP] [OMPT] [amdgpu] [5/8] Implemented device init/fini/load callbacks.Jun 20 2022, 10:45 PM
dhruvachak edited the summary of this revision. (Show Details)
jplehr added a subscriber: jplehr.Dec 1 2022, 8:04 AM
jplehr added inline comments.Dec 1 2022, 8:59 AM
openmp/libomptarget/include/ompt_device_callbacks.h
18

Should this be rather <string> or <cstring>?

openmp/libomptarget/plugins/amdgpu/src/ompt_callback.cpp
30

I'm not sure about this line. I understand it as it may leak memory, and invalidate all OmptDeviceTy pointers that may have been acquired. At least if this is executed more than once (for whatever reason).

jplehr added inline comments.Dec 1 2022, 12:42 PM
openmp/libomptarget/include/ompt_device_callbacks.h
53

In other parts I saw that C-style casts were changed to static_cast or reinterpret_cast.
Potentially stick to these C++ casts instead is preferable.

jplehr updated this revision to Diff 496590.Feb 10 2023, 1:20 PM

Rebase / update

jplehr updated this revision to Diff 496940.Feb 13 2023, 5:31 AM

Fixing build error in Debug configuration

mhalk updated this revision to Diff 499483.Feb 22 2023, 6:49 AM
mhalk added a subscriber: mhalk.

Rebased + related fixes

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

Refactored patch for use with nextgen plugins.

Thanks for moving this forward.

openmp/libomptarget/include/ompt_device_callbacks.h
48

If I recall correctly, methods should be formulated more in active voice, e.g., initializeOmptCallbackDevice, and start with a lower-case letter. I believe this is true for the other methods in this class as well.

136

I think this should be renamed to allocate or so, as the comment suggests that it allocates and does not resize.

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
29 ↗(On Diff #506967)

Probably better use nullptr here.

32 ↗(On Diff #506967)

Potentially we should not allocate if Devices already holds a pointer, i.e., is not nullptr? This looks suspiciously as if we can leak memory here.

38 ↗(On Diff #506967)

Do we want / need to guard against out-of-bounds access here?

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
470–473 ↗(On Diff #506967)

Idk if there is a guideline for it, but I personally would prefer to have this read as /* FileName= */ nullptr, ...

mhalk commandeered this revision.Mar 21 2023, 8:34 AM
mhalk added a reviewer: dhruvachak.
mhalk edited the summary of this revision. (Show Details)Mar 21 2023, 8:36 AM
mhalk added a reviewer: jplehr.

When more changes have accumulated I'll update this diff with the changes I replied to with 'Done'.

openmp/libomptarget/include/ompt_device_callbacks.h
48

Agreed.
If more people request this NFC, I'll gladly incorporate this in the patch stack.
I decided against this, since I wanted to concentrate on the core of the patches and not defer them any longer.

136

Agreed. Done.

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
29 ↗(On Diff #506967)

Done.

32 ↗(On Diff #506967)

Agreed. I'll simply send a DebugPrint when allocation failed.
That is, when: zero or negative NumDevices were requested or when Devices was already allocated / non-nullptr.

38 ↗(On Diff #506967)

I think we should.
Done.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
470–473 ↗(On Diff #506967)

I would be interested in that, too -- in the previous version of these files it was this (nullptr /* FileName= */,) way.
But TBH I don't find that intuitively either, i.e. I would also prefer your proposal.
Any other thoughts or info on this would be appreciated.

dhruvachak added inline comments.Mar 22 2023, 9:52 AM
openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
32 ↗(On Diff #506967)

Why not assert that Devices is nullptr before allocation?

mhalk updated this revision to Diff 507426.Mar 22 2023, 10:38 AM

Patch rework; implemented feedback.

mhalk added inline comments.Mar 22 2023, 10:41 AM
openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
32 ↗(On Diff #506967)

I decided to update the revision, so you can see how I handled things "in the meantime".
(Hopefully, my intermediate changes won't break this, otherwise I'll have to update the whole patch stack.)

dhruvachak added inline comments.Mar 22 2023, 11:25 AM
openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
32 ↗(On Diff #506967)

I would not introduce a new variable for number of devices. I would just add an assert for what we are expecting.

Read the large comment first, this might also result in changes to other patches. The design should match nextgen and it should be a proper part of it, not some attached afterthought.

openmp/libomptarget/include/ompt_device_callbacks.h
142

I don'n understand why there are so many static members and functions everywhere, but alas.

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
30 ↗(On Diff #507426)

Now there are static members, and static non members, I mean, why?

29 ↗(On Diff #506967)

Also above?

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
366 ↗(On Diff #507426)

So, these OmptDevices are stored in 4 or so static members mostly inside OmptDeviceCallbacksTy, and initialized via a member of OmptDeviceCallbacks which is a global object that flies around.

This is really too much.

Let's approach this differently:
One OmptDeviceTy per device of a plugin, right?
If so, make one such object a member of GenericDeviceTy, all that array stuff and 4 functions can be removed, init and deinit are performed via constructor/destructor or init/deinit of the parent.
Funnily, once you do that, the entire OmptDeviceTy code would be moot and all you really put into GenericDeviceTy::(de)init is one conditional to check and call the callback.
You can even make a variadic macro that checks the callback and calls it, forwarding the args, which will simplify the pattern to sth like:

ompt_callback(device_initialize, DeviceNum, Type, reinterpret_cast<ompt_device_t *>(this), doLookup, Documentation);

Then, why do we need OmptDeviceCallbacksTy in the first place?
Shouldn't the callbacks be close to their invocation, e.g., in GenericDeviceTy, etc.

470–473 ↗(On Diff #506967)

/* Name */ Value, which is hopefully what the rest of the code does.

dhruvachak added inline comments.Apr 5 2023, 12:39 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
366 ↗(On Diff #507426)

I agree all or most of those statics can go away. There is a single instance of OmptDeviceCallbacksTy in libomptarget and another instance in the plugin. So those statics are redundant.

About OmptDeviceCallbacksTy, I suggest splitting it up into 2 structs. The first, OmptHostEventsCallbacksTy would contain the host callback function ptrs. The second, OmptDeviceEventsCallbacksTy would contain the 4 device callback function ptrs: load, unload, init, and fini. This is along the lines of the categories already present in omp-tools.h. Then, libomptarget would instantiate only OmptHostEventsCallbacksTy by using the "connect" interface to libomp. Similarly, the plugin would instantiate only OmptDeviceEventsCallbacksTy by using the "connect" interface to libomp. (In the current patch, the plugin connects to libomptarget, that will have to be changed.) The object of type OmptDeviceEventsCallbacksTy can reside in GenericDeviceTy. (Later on, when we introduce support for trace records, we will need new OMPT entry points that the tool can call. Examples: ompt_start_trace, ompt_flush_trace, etc. At that point, another struct OmptDeviceEntryPointsTy holding those entry points can be introduced, residing in GenericDeviceTy. But that's for later.) The object of type OmptHostEventsCallbacksTy can stay as a global object like in current trunk unless we find a better place for it.

dhruvachak added inline comments.Apr 6 2023, 10:47 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
1047 ↗(On Diff #507426)

This will be a problem the way these patches currently are. GenericPluginTy::init() will kick in for multiple plugins, so multiple plugins will initialize the callbacks. Not a problem per se but where it will fail today is during tracking the finalizer in libomptarget. libomptarget is currently set up to track only 1 finalizer.

See openmp/libomptarget/src/OmptCallback.cpp
/// Object that will maintain the finalizer of the plugin
static LibomptargetRtlFinalizer LibOmptTargetRTLFinalizer;

So multiple plugins will have callbacks initialized and the last finalizer will overwrite the rest, not a good thing. (We should actually assert on this condition.)

One solution is to move the call to OmptCallbackInit() within the device-specific plugin implementations, e.g. in AMDGPUPluginTy::initImpl().

mhalk updated this revision to Diff 517238.Apr 26 2023, 10:40 AM

Rebase + implementation of feedback

mhalk added inline comments.Apr 26 2023, 10:59 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
1047 ↗(On Diff #507426)

Thanks and good catch -- I moved the calls to ompt::connectLibrary(); into the specific amdgpu & cuda plugins.
(This comment will be removed in a following diff update. Oversight.)

mhalk updated this revision to Diff 517275.Apr 26 2023, 12:51 PM

Removed debug prints / comments and fixed a problem when no OMPT was initialized.

mhalk updated this revision to Diff 517683.Apr 27 2023, 12:39 PM

Updated to accomodate the changes made in the previous patch.

jdoerfert added inline comments.Apr 27 2023, 3:00 PM
openmp/libomptarget/CMakeLists.txt
70 ↗(On Diff #517683)

?

openmp/libomptarget/include/ompt_connector.h
104 ↗(On Diff #517683)

?

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
35 ↗(On Diff #517683)

Style, here and elsewhere. We use LLVM code style in the plugins, at least outside the C-bindings.

openmp/runtime/src/exports_so.txt
29 ↗(On Diff #517683)

?

openmp/runtime/src/kmp_utility.cpp
410 ↗(On Diff #517683)

?

mhalk updated this revision to Diff 517862.Apr 28 2023, 4:06 AM

Addressed style issues, dreaded whitespace & removed DEBUG_PREFIX testing

mhalk added inline comments.Apr 28 2023, 4:09 AM
openmp/libomptarget/CMakeLists.txt
70 ↗(On Diff #517683)

Had some issues, that was a testing leftover -- sorry!

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
35 ↗(On Diff #517683)

Thanks! Should be addressed (catched three locations).

mhalk updated this revision to Diff 519872.May 5 2023, 8:20 AM
This comment was removed by mhalk.
mhalk updated this revision to Diff 519876.May 5 2023, 8:28 AM

Update + rebase to reflect the landing of https://reviews.llvm.org/D124070

@jdoerfert If there are any issues left or further improvements that should be implemented, please let me know.

dhruvachak added inline comments.May 5 2023, 9:36 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h
27 ↗(On Diff #519876)

Guard it with OMPT_SUPPORT?

753 ↗(On Diff #519876)

So now the size of GenericDeviceTy varies based on whether OMPT support is compiled in or not. I would prefer removing the ifdef here and in the ctor (so that proper init happens) so that the size is the same regardless of cmake config. But I am not requesting this change, would be curious what others feel. I just like it that way since it helps debugging when there is a problem.

760 ↗(On Diff #519876)

Give it a more meaningful name, it's just an atomic boolean now. Some OMPT APIs require an opaque ompt_device_t. In the previous version, we would pass a handle to OmptDevice for those APIs, so that may have to refactored when the time comes.

dhruvachak added inline comments.May 5 2023, 10:49 AM
openmp/libomptarget/src/OmptCallback.cpp
91 ↗(On Diff #519876)

I thought we got rid of these 2 statics.

mhalk added inline comments.May 5 2023, 10:55 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h
27 ↗(On Diff #519876)

Yes, absolutely.

760 ↗(On Diff #519876)

Check w.r.t. "API" and I'd tend to sth. like OmptInitialized, since we're already looking at a "Device" this should convey the fact that we're looking at a boolean that's true when this device's OMPT relatied init has been executed.

dhruvachak added inline comments.May 5 2023, 10:58 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h
760 ↗(On Diff #519876)

Ignore the second part of this comment about ompt_device_t. I see what you are doing already in this patch for the opaque handle and that looks good.

jplehr added a comment.May 9 2023, 4:13 AM

Thanks for moving this forward.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
386 ↗(On Diff #519876)

Nit: I believe this should start w/ capital letter.

448 ↗(On Diff #519876)

captial E

mhalk updated this revision to Diff 521611.May 12 2023, 5:19 AM

Update: Removed statics (libomptarget->plugins). Style.

mhalk added inline comments.May 12 2023, 6:20 AM
openmp/libomptarget/src/OmptCallback.cpp
91 ↗(On Diff #519876)

We wanted to get rid of these statics in openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp
Nevertheless good catch, since those were accidentally re-introduced by a rebase.

AFAIK:
We wanted to keep these here, to make sure the libomp->libomptarget connection stays alive (esp. w.r.t. the result).
But the libomptarget->plugin ones should not be static, since we can have multiple plugins.

mhalk added a comment.Jun 16 2023, 5:53 AM

If there are further requests / improvements I should implement, please let me know.
Otherwise, I'd like to move on with this patch.

Thank you!

mhalk updated this revision to Diff 536256.Jun 30 2023, 7:58 AM

Rebase
Add multiple plugin finalization
Fix cmake not disabling libomptarget OMPT support, when libomp's OMPT support was disabled in the first place

parzt added a subscriber: parzt.Jul 5 2023, 8:12 AM
jplehr accepted this revision.Jul 6 2023, 2:35 AM

I know you did quite a bit of testing, so I assume this works. Just the one nit.
LGTM

openmp/libomptarget/src/OmptCallback.cpp
82 ↗(On Diff #536256)

Should we assert here that LibraryFinalizer is actually nullptr, so we would catch any changes in assumptions about how often something is called early?

This revision is now accepted and ready to land.Jul 6 2023, 2:35 AM
mhalk added a comment.Jul 6 2023, 4:27 AM

Thank you @jplehr, that is much appreciated.

openmp/libomptarget/src/OmptCallback.cpp
82 ↗(On Diff #536256)

Yes, I'll add an assert here -- making sure LibraryFinalizer == nullptr before creating the object.

mhalk updated this revision to Diff 537688.Jul 6 2023, 6:07 AM

Added assert w.r.t. LibraryFinalizer == nullptr

mhalk added a comment.Jul 6 2023, 6:14 AM

@jdoerfert @dreachem *ping*

In case you have any further feedback.

dhruvachak added inline comments.Jul 8 2023, 8:54 AM
openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
476 ↗(On Diff #537688)

Is there a reason you need to pass in the GenericPluginTy here? I don't see it getting used.

470–473 ↗(On Diff #506967)

Yes, that is the guideline. /* Name=*/Value
See https://llvm.org/docs/CodingStandards.html#commenting

openmp/runtime/src/ompt-general.cpp
883 ↗(On Diff #537688)

This looks unrelated. Can you verify whether we really need this for this patch?

mhalk added a comment.Jul 9 2023, 5:00 AM

Thanks @dhruvachak I'll rebase (esp. w.r.t. the deletion of the legacy plugins), then check the upcoming changes and if I don't see problems: submit here.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
476 ↗(On Diff #537688)

Very good catch, this *was* needed initally, when the init and fini was done within the generic plugin.
But this change is not necessary anymore as the patch has undergone a lot of changes since then.

openmp/runtime/src/ompt-general.cpp
883 ↗(On Diff #537688)

Yes, we actually use this in this patch, to provide access to a "callback function lookup by code" (rather than name):
ompt_get_callback_t lookupCallbackByCode
It will be used to bind the actual OMPT callback functions to their declarations.

My idea was: This should provide a lookup with less overhead, than by name (comparing integers vs. strcmp, esp. since the function names are prefixed with ompt_callback_).

mhalk updated this revision to Diff 538422.Jul 9 2023, 6:08 AM

Rebase + reverted signature change of GenericDeviceTy::deinit()

dhruvachak accepted this revision.Jul 9 2023, 10:44 AM

Looks good. Thanks for the changes.