[Openmp-commits] [PATCH] D127365: [OpenMP] [OMPT] [6/8] Added callback support for target data operations, target submit, and target regions.

[Jan-Patrick Lehr via Phabricator via Openmp-commits]

jplehr added a comment.

Some initial comments



================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:101
+  /// external monitoring interface (EMI) callback is registered
+  void OmptCallbackTargetDataOpEmi(
+      ompt_scope_endpoint_t EndPoint, ompt_data_t *TargetTaskData,
----------------
The methods here should probably read in an active voice, e.g., `invokeOmptTargetDataOpEmiCallback` and alike.


================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:219
+    } else if (EndPoint == ompt_scope_begin) {
+      return OmptCallbackTargetSubmit(TargetData->value, RequestedNumTeams,
+                                      IdInterface, HostOpId);
----------------
No `return` needed.


================
Comment at: openmp/libomptarget/src/OmptCallback.cpp:53
+/// Used to create a new correlation id
+static uint64_t id_create() { return unique_id_ticket.fetch_add(1); }
+
----------------
These functions should probably be updated to follow the LLVM naming convention, e.g., `createId`. Idk if this should be done right now, or in an NFC after the OMPT support has landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127365