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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:11
+//
+//===----------------------------------------------------------------------===//
+
----------------
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?


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:99
+                                                                                                                          \
+    /*macro (callback_dispatch,          ompt_callback_dispatch_t,          32)*/ /* dispatch of work                */
+
----------------
Why do we have commented lines here?


================
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);}
+}
+
----------------
Are we sure we don't want to generate these as well?


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:763
+    else                                                                                              \
+      return ompt_multiplex_implementation_status.ompt_##event_name;
+
----------------
Style: Given that the file is new, please clang format it.


================
Comment at: openmp/tools/multiplex/ompt-multiplex.h:899
+//macro to avoid double declaration
+#define ompt_start_tool ompt_multiplex_own_start_tool
+
----------------
I was unable to determine how this macro helps, e.g., why is it better than `OMPT_MULTIPLEX_H`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76012