[Openmp-commits] [PATCH] D76012: [OpenMP][Tool] Header-only multiplexing of OMPT tools

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added inline comments.


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:11
+//
+//===----------------------------------------------------------------------===//
+
----------------
jdoerfert wrote:
> 1) Can we make the file header the same as LLVM files?
> 2) Can we include the idea and short usage + link to the README in the file doxygen comment?
I used the same header as any file in openmp/runtime.
I updated the header as requested.


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:99
+                                                                                                                          \
+    /*macro (callback_dispatch,          ompt_callback_dispatch_t,          32)*/ /* dispatch of work                */
+
----------------
jdoerfert wrote:
> Why do we have commented lines here?
Fair point. I added the implementation for those functions. We cannot test them because these callbacks are not yet implemented in the runtime (mostly libomptarget)


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:318
+    ompt_multiplex_client_callbacks.ompt_callback_cancel(ompt_multiplex_get_client_task_data(task_data),flags,codeptr_ra);}
+}
+
----------------
jdoerfert wrote:
> Are we sure we don't want to generate these as well?
The challenge in generating those functions is the `ompt_multiplex_get_own_task_data` / `ompt_multiplex_get_client_task_data`, which must be applied to all *_data arguments. Such arguments are all over the place.


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:763
+    else                                                                                              \
+      return ompt_multiplex_implementation_status.ompt_##event_name;
+
----------------
jdoerfert wrote:
> Style: Given that the file is new, please clang format it.
This part was actually clang-formated. 
By moving the `;` to the definition of `OMPT_LOAD_CLIENT_FOREACH_OMPT_EVENT`, clang-format now also accepts that definition.


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:899
+//macro to avoid double declaration
+#define ompt_start_tool ompt_multiplex_own_start_tool
+
----------------
jdoerfert wrote:
> I was unable to determine how this macro helps, e.g., why is it better than `OMPT_MULTIPLEX_H`?
This macro renames the `ompt_start_tool` function of the original tool to `ompt_multiplex_own_start_tool`.  
The multiplex header defines `ompt_start_tool` in line 846, which calls the renamed function in line 888.


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

https://reviews.llvm.org/D76012