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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

I think I went over all comments and marked all that are not done yet.

One additional thing that should be doable with a global search-and-replace: This patch uses `if (ompt.enabled)`, `__builtin_expect` and `UNLIKELY` inconsistently. At least for the latter two, we should agree on one. I prefer the macro because this makes it easier to no-op should there be a platform that doesn't support `__builtin_expect` (for whatever reason)



================
Comment at: runtime/src/kmp_gsupport.cpp:1233-1234
                                                                                \
+    IF_OMPT_SUPPORT(if (ompt_enabled.enabled)                                  \
+                        OMPT_STORE_RETURN_ADDRESS(gtid);)                      \
     KMP_DISPATCH_INIT(&loc, gtid, (schedule), lb,                              \
----------------
`OMPT_STORE_RETURN_ADDRESS` has `if (ompt_enabled.enabled)` built-in


================
Comment at: runtime/src/kmp_tasking.cpp:602
       1; // Execute this task immediately, not deferred.
+
   __kmp_task_start(gtid, task, current_task);
----------------
omalyshe wrote:
> Remove extra empty line
That's not done...


================
Comment at: runtime/src/kmp_tasking.cpp:894
                 gtid, loc_ref, KMP_TASK_TO_TASKDATA(task)));
+
   // this routine will provide task to resume
----------------
omalyshe wrote:
> to be removed
Not done yet


================
Comment at: runtime/src/ompt-general.cpp:124
+  ompt_start_tool_t next_tool = NULL;
+  *(void **)(&next_tool) = dlsym(RTLD_NEXT, "ompt_start_tool");
+  if (next_tool)
----------------
Hahnfeld wrote:
> `next_tool = (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool");` looks much more readable IMO
ping


================
Comment at: runtime/src/ompt-general.cpp:57-60
+typedef int (*ompt_initialize_t)(ompt_function_lookup_t lookup,
+                                 struct ompt_fns_t *fns);
+
+typedef void (*ompt_finalize_t)(struct ompt_fns_t *fns);
----------------
I think I asked this some days ago: Aren't these defined in `ompt.h`?

EDIT: Ah, in the TR6 patch!


================
Comment at: runtime/src/ompt-general.cpp:219
+  // Try tool-libraries-var ICV
+  const char *tool_libs = getenv("OMP_TOOL_LIBRARIES");
+  if (tool_libs) {
----------------
Hasn't this been parsed to `__kmp_tool_libraries`?


================
Comment at: runtime/src/ompt-general.cpp:228
+      if (h) {
+        *(void **)(&start_tool) = dlsym(h, "ompt_start_tool");
+#elif KMP_OS_WINDOWS
----------------
`start_tool = (ompt_start_tool_t)dlsym(RTLD_NEXT, "ompt_start_tool");` as above


================
Comment at: runtime/src/ompt-internal.h:34
+
+typedef struct kmp_taskdata kmp_taskdata_t;
+
----------------
Why is this here?


https://reviews.llvm.org/D38185