[Openmp-commits] [PATCH] D134396: [OpenMP][libomptarget] New plugin infrastructure and new CUDA plugin

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 accepted this revision.
jhuber6 added a comment.
This revision is now accepted and ready to land.

LG overall with a few more nits.

I tested the x86_64 plugins locally and it works well. I really like how much cleaner the interfaces and error handling are overall. Thanks for sticking with this.



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:162
+               ptrdiff_t *ArgOffsets, int32_t NumArgs, int32_t NumTeamsClause,
+               int32_t ThreadLimitClause, int32_t LoopTripCount,
+               AsyncInfoWrapperTy &AsyncInfoWrapper) const;
----------------
Why do we use `int32_t` for the loop trip count when the interface uses `uint64_t`? I think it's totally possible someone would have a trip-count > 3 billion or so.


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/CMakeLists.txt:92
+install(TARGETS omptarget.rtl.cuda.nextgen LIBRARY DESTINATION "${OPENMP_INSTALL_LIBDIR}")
+set_target_properties(omptarget.rtl.cuda.nextgen PROPERTIES 
+  INSTALL_RPATH "$ORIGIN" BUILD_RPATH "$ORIGIN:${CMAKE_CURRENT_BINARY_DIR}/..")
----------------
Just FYI, I made a change to this on all the plugins in D136365 that should be copied here. This prevents the plugins from having their symbols preempted by other plugins. This is the safest approach as each plugin defines the same symbols but with different values.


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:357
+      // Consume the error since it is acceptable to fail.
+      consumeError(std::move(Err));
+      // In some cases the execution mode is not included, so use the default.
----------------
if the debugging message fires we move the `Error` twice here.


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:452-477
+    auto Err = Plugin::success();
+    consumeError(std::move(Err));
+
+    if (Err = setContext()) {
+      REPORT("Failure to free memory: %s\n", toString(std::move(Err)).data());
+      return OFFLOAD_FAIL;
+    }
----------------
We should probably just create a new `Error` for each line here. Either that or replace it with `if ((Err = setContext()))` or else the linters will complain.


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:862-866
+    } else if (Res == CUDA_ERROR_NO_DEVICE) {
+      // Do not initialize if there are no devices.
+      DP("There are no devices supporting CUDA.\n");
+      return;
+    }
----------------
Nit: no else after return.


================
Comment at: openmp/libomptarget/plugins-nextgen/exports:3-37
+    __tgt_rtl_init_plugin;
+    __tgt_rtl_deinit_plugin;
+    __tgt_rtl_is_valid_binary;
+    __tgt_rtl_is_valid_binary_info;
+    __tgt_rtl_is_data_exchangable;
+    __tgt_rtl_number_of_devices;
+    __tgt_rtl_init_requires;
----------------
I changes the `exports` file upstream recently to use a wildcard. It should be easier this way.


================
Comment at: openmp/libomptarget/plugins-nextgen/generic-elf-64bit/src/rtl.cpp:28
+#include "llvm/Support/DynamicLibrary.h"
+
+// The number of devices in this plugin.
----------------
In the previous implementation, we used some definitions so that the debug messages indicated which plugin was executing. Right now I just see messages like this.
```
PluginInterface --> Launching kernel __omp_offloading_10302_4741dfd_main_l5 with 1 blocks and 1 threads in Generic mode
```

I'm thinking we should be able to add a name for each subclass of the generic plugin to use for the debugging messages. I think it would also be nice to move away from the `DP(` macros if possible.

This isn't really high priority so I wouldn't worry about it for now.


================
Comment at: openmp/libomptarget/plugins-nextgen/generic-elf-64bit/src/rtl.cpp:30
+// The number of devices in this plugin.
+#define NUM_DEVICES 4
+
----------------
Unrelated, but does anyone know why we still set this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134396