Introduce an plugin interface to query the status of an event.
Also record whether a device support events or not.
Details
- Reviewers
jdoerfert tianshilei1992
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
openmp/libomptarget/include/omptarget.h | ||
---|---|---|
184 | I'm not sure about the naming convention for this enum type name. |
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. |
openmp/libomptarget/plugins/cuda/src/rtl.cpp | ||
---|---|---|
162 | Change macro to enum takes a bit longer. So just keep using macro for the moment. |
openmp/libomptarget/include/omptargetplugin.h | ||
---|---|---|
155 | 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 | ||
25 | 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.
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. |
openmp/libomptarget/include/omptargetplugin.h | ||
---|---|---|
155 | query_event is not mandatory nor sync_event I guess. You can choose one way or another. | |
openmp/libomptarget/src/device.cpp | ||
25 |
openmp/libomptarget/include/omptargetplugin.h | ||
---|---|---|
155 |
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 | ||
25 |
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. |
openmp/libomptarget/include/omptargetplugin.h | ||
---|---|---|
155 | 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 | ||
25 | 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. 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. |
openmp/libomptarget/include/omptargetplugin.h | ||
---|---|---|
155 | 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 | ||
25 |
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.
It's as good (or bad) as using a Boolean variable.
I don't doubt that. I just expect to have the feature enabled, and also work like existing code. |
I'm not sure about the naming convention for this enum type name.
Also if this is the right location to add this enum.