[Openmp-commits] [PATCH] D38185: Implementation of OMPT as specified in OpenMP 5.0 Preview 1

[Hansang Bae via Phabricator via Openmp-commits]

hbae added a comment.

Just a few small things from me.



================
Comment at: runtime/src/exports_so.txt:29
+        ompt_start_tool;     # OMPT start interface
         ompt_control;        # OMPT control interface
 
----------------
This is not necessary since we have omp_control_tool now.


================
Comment at: runtime/src/include/50/ompt.h.var:691
 #endif
 void ompt_control(
     uint64_t command,
----------------
Do we still need this function?


================
Comment at: runtime/src/kmp_csupport.cpp:2994
+        ompt_mutex_nest_lock, omp_lock_hint_none,
+        0, // TODO for intel: specify impl
+        (ompt_wait_id_t)user_lock, codeptr);
----------------
omalyshe wrote:
> Should __ompt_get_mutex_impl_type() be called?
Yes, it should be __ompt_get_mutex_impl_type(user_lock)


================
Comment at: runtime/src/kmp_tasking.cpp:458
+//   Initialize OMPT fields maintained by a task. This will only be called after
+//   ompt_tool, so we already know whether ompt is enabled or not.
+
----------------
ompt_tool -> ompt_start_tool


================
Comment at: runtime/src/kmp_tasking.cpp:462
+// The calls to __ompt_task_init already have the ompt_enabled condition.
+//  if (__builtin_expect(ompt_enabled.enabled,0)) {
+  task->ompt_task_info.task_data.value = 0;
----------------
Remove the commented if condition.


================
Comment at: runtime/src/kmp_tasking.cpp:555
+    ompt_task_info_t *parent_info = &(current_task->ompt_task_info);
+    ompt_data_t task_data = ompt_data_none;
+    ompt_callbacks.ompt_callback(ompt_callback_task_create)(
----------------
task_data is never used.


https://reviews.llvm.org/D38185