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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109017