[Openmp-commits] [PATCH] D136278: [OpenMP] Add distributed plugin based on MPI

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

Thanks for putting this up for review. It's a lot and I think we should start with a presentation in our weekly meeting. I also would like you to take a look at the new plugin infrastructure that is under review. My hope would be that we target that one right away and avoid lots of duplication. Some of the features you build here might be useful to be generalized anyway.
One initial question I have is about the "main" and "on device" stuff, but I'll ask it for sure in our meeting. Some drive by comments below.



================
Comment at: openmp/libomptarget/plugins/mpi/src/EventSystem.cpp:42
+// values. Every single one of them can be tuned by an environment variable with
+// the following name pattern: OMPCLUSTER_VAR_NAME.
+namespace config {
----------------
We certainly want to rename this.


================
Comment at: openmp/libomptarget/plugins/mpi/src/EventSystem.cpp:80
+  assertm(false, "Every enum value must be checked on the switch above.");
+  return nullptr;
+}
----------------
I would recommend against custom "assert" macros, the `&&` notation is literally not more to type. That said, `assert(0)` is not helpful. Either make it an `llvm_unreachable` or a `printf + __builtin_trap`.


================
Comment at: openmp/libomptarget/src/rtl.cpp:30
 static const char *RTLNames[] = {
+    /* MPI target           */ "libomptarget.rtl.mpi.so",
     /* PowerPC target       */ "libomptarget.rtl.ppc64.so",
----------------
Bottom of the list, please.


================
Comment at: openmp/libomptarget/src/rtl.cpp:179
-    *((void **)&R.deinit_plugin) =
-        DynLibrary->getAddressOfSymbol("__tgt_rtl_deinit_plugin");
     *((void **)&R.is_valid_binary_info) =
----------------
Some of these changes seam unrelated and should be split of. E.g., calling deinit if present seems reasonable for all plugins.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136278