Page MenuHomePhabricator

[OpenMP][libomptarget] Add event query plugin API
Needs ReviewPublic

Authored by ye-luo on Aug 31 2021, 12:56 PM.

Details

Summary

Introduce an plugin interface to query the status of an event.
Also record whether a device support events or not.

Diff Detail

Event Timeline

ye-luo created this revision.Aug 31 2021, 12:56 PM
ye-luo requested review of this revision.Aug 31 2021, 12:56 PM
ye-luo added inline comments.Aug 31 2021, 12:58 PM
openmp/libomptarget/include/omptarget.h
179

I'm not sure about the naming convention for this enum type name.
Also if this is the right location to add this enum.

What is the use case of the API and of the new member?

What is the use case of the API and of the new member?

The new member is needed for D107656 to opt-in using events for synchronization if event is supported.
D107656 also need query_event to decide if a task should be resumed.

ye-luo updated this revision to Diff 370477.Sep 2 2021, 9:13 PM

I think we need something like this for sure soon. @tianshilei1992 thoughts?

tianshilei1992 added inline comments.Sep 3 2021, 10:57 AM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
162

IMO, it's better to extend the return value, aka OFFLOAD_SUCCESS, instead of using this method. Currently we have two status, 0, and ~0. We can add plenty of status.

ye-luo added inline comments.Sep 4 2021, 1:07 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
162

Add D109277 to warmup and detect potential issues.

ye-luo updated this revision to Diff 371248.Sep 7 2021, 8:28 PM

Expand macro for the moment.

Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2021, 8:28 PM
ye-luo added inline comments.Sep 7 2021, 8:32 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
162

Change macro to enum takes a bit longer. So just keep using macro for the moment.

tianshilei1992 added inline comments.Sep 10 2021, 9:58 AM
openmp/libomptarget/include/omptargetplugin.h
155 ↗(On Diff #371248)

Actually, I don't see query_event is mandatory here. We don't have to query it before using it. From my understanding, query_event is more like an option that we may want to do something if the event is still not fulfilled.

openmp/libomptarget/src/device.cpp
49

HasEventSupport is not necessary here. If any event related interface is nullptr, it is not supported, and we will not call it. There are basically two options to implement them.

  • Set a Boolean variable when DeviceTy is initialized. Every time we call an underlying RTLInfoTy function, we check the Boolean variable. That's what you proposed here.
  • Every time we call an underlying RTLInfoTy function, we check if the function pointer is nulltpr. That's we currently used.

I didn't see any issue from what we are using. As long as the behavior is consistent, they are same. Setting a Boolean variable will not save any branch instruction.

ye-luo added inline comments.Sep 10 2021, 10:32 AM
openmp/libomptarget/include/omptargetplugin.h
155 ↗(On Diff #371248)

query_event is not mandatory nor sync_event I guess. You can choose one way or another.
In some cases if we use wait_event, neither query_event nor sync_event may be needed.
What give you the impression that query_event is mandatory? Do I need to change something here to make things more clear?

openmp/libomptarget/src/device.cpp
49

Currently when event is not supported, event APIs are still called and get OFFLOAD_SUCCESS. I find it hard to follow the code for example in D104418.
So prefer a boolean to skip large chunk of calls to any event related APIs. For example in D107656, I can directly decide to use event or queue.

openmp/libomptarget/include/omptargetplugin.h
155 ↗(On Diff #371248)

query_event is not mandatory nor sync_event I guess. You can choose one way or another.

IIRC, you mentioned somewhere previously that, stream synchronization is just spinning, which really hurts performance. If sync_event is something like conditional variable, we can use it to replace stream synchronization. That's what I expected when I wrote those lines. If it is not the case, we could add "(optional)" or add "if needed" or something like that.

openmp/libomptarget/src/device.cpp
49

Currently when event is not supported, event APIs are still called and get OFFLOAD_SUCCESS. I find it hard to follow the code for example in D104418.

Returning OFFLOAD_SUCCESS is like a convention we are using in libomptarget. When an optional feature (basically a function pointer) is not supported, it should behave like it doesn't break anything by returning OFFLOAD_SUCCESS. Well, of course we could use a Boolean variable for each feature, but then we will have a bunch of Boolean variables, which is unnecessary.
Sometime ago I also thought to query what features a device (plugin) can support, and store them in bit set. In this way, when user code has require, we know what feature is supported and which is not immediately. From my perspective, that's the right way to do that.
So back to your code, you could still check if the corresponding function pointer is nullptr. All data members are public, so there is nothing stopping you.

ye-luo added inline comments.Sep 10 2021, 11:23 AM
openmp/libomptarget/include/omptargetplugin.h
155 ↗(On Diff #371248)

When using event API to achieve synchronization, libomptarget can choose query_event or sync_event depending on the use case. or event call __tgt_rtl_wait_event and sync the stream.

When implementing event related APIs in a plugin, all APIs are required. No holes should be left. If we really have to make certain API optional. Then some fine-grain flags are need to indicate a specific API not supported. I think we can do such change when a need shows up.

openmp/libomptarget/src/device.cpp
49

Returning OFFLOAD_SUCCESS is not the issue. The issue is what is the expected behavior of these APIs when events are not supported. right now it sounds like undefined behaviors and thus I'd like to skip calling them. Codes are also much more readable.

> So back to your code, you could still check if the corresponding function pointer is nullptr. All data members are public, so there is nothing stopping you.
I feel this worse than a boolean. All function pointer member being public is already fishy. Since the change of unique_ptr, I can actually mark the boolean const and it is set by the constructor only.

Using bitset or boolean can be done separately. using "requires" can be a separate topic. right now, I think we can just handle the scenario of missing event support of plugins inside lbiomptarget.

tianshilei1992 added inline comments.Sep 13 2021, 1:22 PM
openmp/libomptarget/include/omptargetplugin.h
155 ↗(On Diff #371248)

I mean, the "optional" should be like, the function is not mandatory to be called for the completeness of the whole event mechanism. For example, we could just create event and set dependence on it. We don't have to synchronize it or query it.

openmp/libomptarget/src/device.cpp
49

Returning OFFLOAD_SUCCESS is not the issue. The issue is what is the expected behavior of these APIs when events are not supported. right now it sounds like undefined behaviors and thus I'd like to skip calling them. Codes are also much more readable.

Like I said before, the reason the optional features (interfaces) are called optional is that even if they are not there, they should be transparent and not break anything. It is defined behavior. What you are trying to do is to introduce inconsistency: for some of other optional APIs, when they are not supported, it returns success. For this specific set of APIs, when they are not supported, they are not called.

I feel this worse than a boolean. All function pointer member being public is already fishy. Since the change of unique_ptr, I can actually mark the boolean const and it is set by the constructor only.

It's as good (or bad) as using a Boolean variable.

Using bitset or boolean can be done separately. using "requires" can be a separate topic. right now, I think we can just handle the scenario of missing event support of plugins inside lbiomptarget.

I don't doubt that. I just expect to have the feature enabled, and also work like existing code.