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

[Shilei Tian via Phabricator via Openmp-commits]

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:
> > 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.


================
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:
> > `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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109017