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

[mhalk]

Summary

Added support in the generic plugin to invoke registered callbacks.

Depends on D124070

Patch from John Mellor-Crummey <[e-mail]>
(With contributions from Dhruva Chakrabarti <[e-mail]>)

[jplehr]

openmp/libomptarget/include/ompt_device_callbacks.h

18

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

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

30 ↗

(On Diff #438557)

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]

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]

Rebase / update

[jplehr]

Fixing build error in Debug configuration

[jplehr]

Rebased

[mhalk]

Rebased + related fixes

[mhalk]

Refactored patch for use with nextgen plugins.

[jplehr]

Thanks for moving this forward.

openmp/libomptarget/include/ompt_device_callbacks.h

52

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.

141

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

30

Probably better use nullptr here.

33

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.

39

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

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

498–501

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

[mhalk]

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

52

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.

141

Agreed. Done.

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp

30

Done.

33

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.

39

I think we should.
Done.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

498–501

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]

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp

33

Why not assert that Devices is nullptr before allocation?

[mhalk]

Patch rework; implemented feedback.

[mhalk]

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp

33

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]

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp

33

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

[jdoerfert]

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

149

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

Also above?

31

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

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

389

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.

498–501

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

[dhruvachak]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

389

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]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

1085

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]

Rebase + implementation of feedback

[mhalk]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

1085

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]

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

[mhalk]

Updated to accomodate the changes made in the previous patch.

[jdoerfert]

openmp/libomptarget/CMakeLists.txt

70 ↗

(On Diff #517683)

?

openmp/libomptarget/include/OmptConnector.h

103–104

?

openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp

35

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]

Addressed style issues, dreaded whitespace & removed DEBUG_PREFIX testing

[mhalk]

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

Thanks! Should be addressed (catched three locations).

[mhalk]

This comment was removed by mhalk.

[mhalk]

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]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h

27

Guard it with OMPT_SUPPORT?

756

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.

763

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]

openmp/libomptarget/src/OmptCallback.cpp

91

I thought we got rid of these 2 statics.

[mhalk]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h

27

Yes, absolutely.

763

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]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h

763

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]

Thanks for moving this forward.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

389

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

451

captial E

[mhalk]

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

[mhalk]

openmp/libomptarget/src/OmptCallback.cpp

91

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.