[Openmp-commits] [PATCH] D109017: [OpenMP][libomptarget] Add event query plugin API

Mon Sep 13 13:22:34 PDT 2021

tianshilei1992 added inline comments.

================
Comment at: openmp/libomptarget/include/omptargetplugin.h:155
+// 3) Call __tgt_rtl_query_event to query the status of all work currently
+// captured by an event
+// 4) Call __tgt_rtl_wait_event to set dependence on the event. Whether the
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > tianshilei1992 wrote:
> > > > 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.
> > > 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?
> > > 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.
> 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.
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.

================
Comment at: openmp/libomptarget/src/device.cpp:25
+      HasPendingGlobals(false),
+      HasEventSupport(RTL->create_event ? true : false), HostDataToTargetMap(),
+      PendingCtorsDtors(), ShadowPtrMap(), DataMapMtx(), PendingGlobalsMtx(),
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > tianshilei1992 wrote:
> > > > `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.
> > > 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.
> > > 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.
> 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.
> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109017/new/

https://reviews.llvm.org/D109017