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

[Olga Malysheva via Phabricator via Openmp-commits]

omalyshe added inline comments.


================
Comment at: runtime/src/kmp_barrier.cpp:1912
+          ompt_enabled.ompt_callback_implicit_task) {
+        // don't access *pteam here: it may have already been freed
+        // by the master thread behind the barrier (possible race)
----------------
Comment is unclear; what is pteam?


================
Comment at: runtime/src/kmp_csupport.cpp:315
 #endif
+
     __kmp_join_call(loc, gtid
----------------
Remove empty line


================
Comment at: runtime/src/kmp_csupport.cpp:656
+#if OMPT_SUPPORT && OMPT_OPTIONAL
+  kmp_int32 global_tid = __kmp_entry_gtid();
+  if (ompt_enabled.ompt_callback_flush) {
----------------
Remove unused variable.


================
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);
----------------
Should __ompt_get_mutex_impl_type() be called?


================
Comment at: runtime/src/kmp_dispatch.cpp:1235
 
-#if OMPT_SUPPORT && OMPT_TRACE
-  if (ompt_enabled && ompt_callbacks.ompt_callback(ompt_event_loop_begin)) {
+// TODO for intel: need to be able to distinguish between sections and loops for
+// ompt callback
----------------
The comment is not relevant any more


================
Comment at: runtime/src/kmp_dispatch.cpp:2544
   KMP_DEBUG_ASSERT(__kmp_init_serial);
+#if OMPT_SUPPORT
+  OMPT_STORE_RETURN_ADDRESS(gtid);
----------------
Should it be  #if OMPT_SUPPORT && OMPT_OPTIONAL in all occurrences below?


================
Comment at: runtime/src/kmp_global.cpp:309
 
+#if OMP_50_ENABLED && LIBOMP_OMPT_SUPPORT
+char const *__kmp_tool_libraries = NULL;
----------------
LIBOMP_OMPT_SUPPORT -> OMPT_SUPPORT


================
Comment at: runtime/src/kmp_gsupport.cpp:36
   KA_TRACE(20, ("GOMP_barrier: T#%d\n", gtid));
-#if OMPT_SUPPORT && OMPT_TRACE
+#if OMPT_SUPPORT
   ompt_frame_t *ompt_frame;
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:45
   __kmpc_barrier(&loc, gtid);
+#if OMPT_SUPPORT
+  if (ompt_enabled.enabled) {
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:67
   KA_TRACE(20, ("GOMP_critical_start: T#%d\n", gtid));
+#if OMPT_SUPPORT
+  OMPT_STORE_RETURN_ADDRESS(gtid);
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:77
   KA_TRACE(20, ("GOMP_critical_end: T#%d\n", gtid));
+#if OMPT_SUPPORT
+  OMPT_STORE_RETURN_ADDRESS(gtid);
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:184
+
+#if OMPT_SUPPORT
+  ompt_frame_t *ompt_frame;
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:197
   retval = __kmp_team_from_gtid(gtid)->t.t_copypriv_data;
+#if OMPT_SUPPORT
+  if (ompt_enabled.enabled) {
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:220
   __kmp_team_from_gtid(gtid)->t.t_copypriv_data = data;
+#if OMPT_SUPPORT
+  ompt_frame_t *ompt_frame;
----------------
Should be under OMPT_OPTIONAL and more others below?


================
Comment at: runtime/src/kmp_gsupport.cpp:256
   KA_TRACE(20, ("GOMP_ordered_start: T#%d\n", gtid));
+#if OMPT_SUPPORT
+  OMPT_STORE_RETURN_ADDRESS(gtid);
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:645
 
+#if OMPT_SUPPORT
+  ompt_frame_t *ompt_frame;
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_gsupport.cpp:833
 
 #if OMPT_SUPPORT
 
----------------
Should be under OMPT_OPTIONAL?


================
Comment at: runtime/src/kmp_runtime.cpp:1394
+                            &ompt_parallel_data, codeptr);
+    // exit_runtime_p =
+    //     &(lw_taskteam.ompt_task_info.frame.exit_runtime_frame);
----------------
Commented code should be cleaned-up as well as unused variables dummy and exit_runtime_p above


https://reviews.llvm.org/D38185