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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo 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
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109017