[Openmp-commits] [PATCH] D138389: [OpenMP][libomptarget] Add AMDGPU NextGen plugin with asynchronous behavior

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D138389#3942377 <https://reviews.llvm.org/D138389#3942377>, @JonChesterfield wrote:

> There's a lot of code here. Some of it very familiar, in copy/paste fashion from the current plugin. Some of it quite subtle in terms of whether it's going to work or not. Not an easy thing to review successfully in one block.

I fondly remember when we merged the AMD plugin, which is to this day full of dead code... That said, @kevinsala, everything that is not AMD related should be split off. Make one ore more pre-commits to update interfaces etc. of the generic parts.

> How was this tested?

We need a configuration for our regression tests. I thought we had one (or a patch) already. @kevinsala @jhuber6

> What's the intended lifetime plan for new and old plugins (it looks like cuda's old one is still with us)?

The next release will have both. After we branch we'll delete the old plugins. Like we did with the runtime.



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.cpp:136
+  // Setup the global symbol's address and size.
+  return getGlobalMetadataFromELF(Image, **SymOrErr, **SecOrErr, ImageGlobal);
 }
----------------
@kevinsala: Split these changes into a separate pre-commit please.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:570
 int32_t __tgt_rtl_deinit_plugin() {
-  auto Err = Plugin::deinit();
+  auto Err = Plugin::deinitIfNeeded();
   if (Err)
----------------
@kevinsala: Split these changes into a separate pre-commit please.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:764
       for (uint32_t I = NewSize; I < OldSize; ++I) {
-        if (auto Err = ResourcePool[I].destroy())
+        if (auto Err = ResourcePool[I].destroy(Device))
           return Err;
----------------
@kevinsala: Split these changes into a separate pre-commit please.


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:1004
+  return new CUDAGlobalHandlerTy();
 }
 
----------------
@kevinsala: Split these changes into a separate pre-commit please.


================
Comment at: openmp/libomptarget/plugins-nextgen/generic-elf-64bit/src/rtl.cpp:371
+  return new GenELF64GlobalHandlerTy();
 }
 
----------------
@kevinsala: Split these changes into a separate pre-commit please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138389