[Openmp-commits] [PATCH] D113728: [libomptarget] [amdgpu] Foundation for OMPT target callback support

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

I commented on the most severe issues I could find. Since I don't know enough about the OMPT inner workings it is mostly style and general code comments.
That said, the lack of (doxygen) documentation is not a great sign. There is not much being explained in the code either. I fear we are piling up more and more code less and less people understand without reasonable means for new people to get into it. For now, I'm fine with this but @dhruvachak you should consider spending some time soon documenting the methods and non-trivial code pieces.



================
Comment at: openmp/libomptarget/include/ompt-connector.h:37
+
+#define stringify(s) #s
+
----------------
This is not something we should expose in global namespace especially as it is not used here and has a rather generic name.


================
Comment at: openmp/libomptarget/include/ompt-connector.h:55
+// it should be declared within the constructor function to ensure that
+// the class is initialized before it's methods are used
+//----------------------------------------------------------------------------
----------------
Nit: Try to make comments proper sentences incl. punctuation and such.


================
Comment at: openmp/libomptarget/include/ompt-connector.h:92
+  std::string library_connect_routine;
+};
+
----------------
I would have used the LLVM coding convention in these new parts, it's a missed opportunity IMHO.

Early exists should be preferred over indention, e.g., `if (is_initialized) return;`

We should at least remove `;` after methods (here and elsewhere).

Clang format all the code.


================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:2
+//=== ompt_device_callbacks.h - Target independent OpenMP target RTL -- C++
+//-===//
+//
----------------
Nit: Style


================
Comment at: openmp/libomptarget/include/omptarget.h:351
 
-void __kmpc_push_target_tripcount(int64_t device_id, uint64_t loop_tripcount);
+void __kmpc_push_target_tripcount(ident_t *loc, int64_t device_id,
+                                  uint64_t loop_tripcount);
----------------
JonChesterfield wrote:
> I thought these functions were ABI stable, i.e. we can't change them
What Jon said. Also, this method is effectively deprecated anyway, the one below is used even though we should not explicitly push the trip count in the first place.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/ompt_callback.cpp:127
+  ompt_result.tool_data.value = 0;
+  ;
+
----------------
These stray `;` appear a few times. Will trigger warnings and should be removed.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1835
+  OMPT_IF_ENABLED(
+      std::string ompt_gpu_type("AMD "); ompt_gpu_type += GetInfoName;
+      const char *type = ompt_gpu_type.c_str();
----------------
Clang format these things, consider `OMPT_IF_ENABLED({ ... })`.
Also all the others that span more than one line/statement.


================
Comment at: openmp/libomptarget/src/device.cpp:459
+  return tgt_ptr;
 }
 
----------------
These functions use LLVM coding style, please don't mix it with the C style other places use.

The pattern here is repetitive and I am unsure if it would not make it much easier to use a RAII object instead.

Comments apply to the rest of the file too.


================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:58
+  ompt_finalize_t fn;
+};
+
----------------
assertions should have messages, use `nullptr` for a pointer not 0.


================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:120
+void OmptInterface::ompt_state_clear(void) {
+  ompt_state_set_helper(0, 0, 0, _state);
+}
----------------
what are those 0s? Use `/* enter_frame */ nullptr, ...`


================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:380
+ompt_device *ompt_device_callbacks_t::lookup_device(int device_num) {
+  assert(0 && "Lookup device should be invoked in the plugin");
+  return nullptr;
----------------
Please no assert(0). In release builds this will silently do something while it arguably should not.
print the error, then explicitly abort, exit, trap, ...


================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:422
+  DP("OMPT: Leave libomptarget_ompt_connect\n");
+}
+}
----------------
technically this is not a reserved name. I doubt it will clash with the user names but it could for all we know. Any chance we can make it a reserved name instead? ompx_ omp_ (ompt_?) __libomptarget ...


================
Comment at: openmp/libomptarget/src/ompt_callback.h:105
+  void *_codeptr_ra;
+  int _state;
+};
----------------
Yet another naming scheme?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:32
+#define OMPT_IF_ENABLED(stmts)
+#endif
+
----------------
This seems something worth refactoring, third time I see it.


================
Comment at: openmp/libomptarget/test/ompt/callbacks.h:71
+                                        const void *codeptr_ra) {
+  assert(0 && "Target map callback is unimplemented");
+}
----------------
Please no assert(0). In release builds this will silently do something while it arguably should not.
print the error, then explicitly abort, exit, trap, ...


================
Comment at: openmp/libomptarget/test/ompt/callbacks.h:125
+                                            const void *codeptr_ra) {
+  assert(0 && "Target map emi callback is unimplemented");
+}
----------------
Please no assert(0). In release builds this will silently do something while it arguably should not.
print the error, then explicitly abort, exit, trap, ...


================
Comment at: openmp/runtime/src/ompt-internal.h:97
   void *idle_frame;
 } ompt_thread_info_t;
 
----------------
Is this something we need to keep stable?


================
Comment at: openmp/runtime/src/ompt-specific.cpp:272
+  lwt->ompt_task_info.frame.exit_frame_flags = 0;
+  ;
   lwt->ompt_task_info.scheduling_parent = NULL;
----------------
Here and above, stray `;` again. Will cause warnings (=problems) eventually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728