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

[Shilei Tian via Phabricator via Openmp-commits]

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

LG w/ some nits.



================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:26
 #include "DeviceEnvironment.h"
+#include "omptarget.h"
 #include "omptargetplugin.h"
----------------
jdoerfert wrote:
> tianshilei1992 wrote:
> > Why do we want `omptarget.h` here?
> Because my clang complete things we use it (probably for some types).
`omptargetplugin.h` includes `omptarget.h` already, but it's a good practice to include what we need. Good to keep it.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:590
 
+    assert(InitializedFlags[DeviceId] == false && "Reinitializing device!");
+    InitializedFlags[DeviceId] = true;
----------------
jdoerfert wrote:
> 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.
I agree. No problem. I mean, we need to do guard it. If not in this patch, then we do it in another patch.


================
Comment at: openmp/libomptarget/src/device.cpp:479
 
+void DeviceTy::deinit() {
+  if (RTL->deinit_device)
----------------
jdoerfert wrote:
> tianshilei1992 wrote:
> > Who calls it?
> Nobody anymore, probably. Was called but I removed it. We would call it for hard_shutdown though.
We can call it in the deinit function emitted by clang in another patch once we land it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120089