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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:26
 #include "DeviceEnvironment.h"
+#include "omptarget.h"
 #include "omptargetplugin.h"
----------------
Why do we want `omptarget.h` here?


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:590
 
+    assert(InitializedFlags[DeviceId] == false && "Reinitializing device!");
+    InitializedFlags[DeviceId] = true;
----------------
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.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:767
+    // deinitialized.
+    if (std::none_of(InitializedFlags.begin(), InitializedFlags.end(),
+                     [](bool IsInitialized) { return IsInitialized; }))
----------------
This `none_of` is useless here because `InitializedFlags[DeviceId]` is reset beforehand.


================
Comment at: openmp/libomptarget/src/device.cpp:479
 
+void DeviceTy::deinit() {
+  if (RTL->deinit_device)
----------------
Who calls it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120089