[Openmp-commits] [PATCH] D120089: [OpenMP] Explicitly deinitialize device resources

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:26
 #include "DeviceEnvironment.h"
+#include "omptarget.h"
 #include "omptargetplugin.h"
----------------
tianshilei1992 wrote:
> Why do we want `omptarget.h` here?
Because my clang complete things we use it (probably for some types).


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:590
 
+    assert(InitializedFlags[DeviceId] == false && "Reinitializing device!");
+    InitializedFlags[DeviceId] = true;
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > tianshilei1992 wrote:
> > > Actually this could have data race. Say if multiple threads are trying to initialize it at the same time. I don't remember we have the guard in `libomptarget`. We could potentially use `std::vector<std::once_flag>` instead.
> > This is something libomptarget should handle. There is little reason N plugins should do it and also little reason to assume plugins should allow concurrent initialization of the same device. That said, `DeviceTy::init()` in `libomptarget` is guarded by a `std::call_once` which should do the trick just fine.
> I'm fine with that, as long as we really do it.
If we wouldn't, how is this patch making it worse? All the assignments below would be racy as well.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:767
+    // deinitialized.
+    if (std::none_of(InitializedFlags.begin(), InitializedFlags.end(),
+                     [](bool IsInitialized) { return IsInitialized; }))
----------------
tianshilei1992 wrote:
> tianshilei1992 wrote:
> > This `none_of` is useless here because `InitializedFlags[DeviceId]` is reset beforehand.
> Oh, `none_of`, nvm.
This is going to be replaced in the follow up when we have one event pool per device (which we need).


================
Comment at: openmp/libomptarget/src/device.cpp:479
 
+void DeviceTy::deinit() {
+  if (RTL->deinit_device)
----------------
tianshilei1992 wrote:
> Who calls it?
Nobody anymore, probably. Was called but I removed it. We would call it for hard_shutdown though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120089