[Openmp-commits] [PATCH] D142249: [openmp] Workaround for HSA in issue 60119

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

In D142249#4070864 <https://reviews.llvm.org/D142249#4070864>, @Maetveis wrote:

> Do we care about de-registration happening during init? It does not happen in the reproducer case when the dynamic loader loads the user libraries, but I think it could happen if one of the libraries HSA loads does a `dlopen` and `dlclose` in its constructor on a third library that's using OpenMP. Typed out like that it sounds somewhat ridiculous and hypothetical, so I'm fine without.

Interesting question. We should have a test cases with a target region in a constructor as well, though I think the current registration system will make that work as long as the user heeds the priority < 100 is reserved contract. We're probably safe from arbitrary calls into libomptarget being written by the user as if they're doing that they have something specific in mind.

Your scenario would break I think. Likewise, a target region in a constructor in a dlopened library will (I think, the graph is getting large enough to make a whiteboard seem sensible) try to use libomptarget before it has managed to construct itself and also break.

I think the right response to such situations is to ignore those edges - noone is currently doing that as they would have started complaining around the 5.3 release if they were, HSA should stop doing this dlopen dance by the next release, and at that point when someone brings a repro along the lines of "I'm doing openmp offloading before main and it doesn't work with rocm 5.4" we'll be able to ask them to upgrade. So it's my hope that the failure mode you've described is latent and will remain so. Code to work around it would tend to stay in tree forever.



================
Comment at: openmp/libomptarget/include/device.h:549-554
+    // Only called by libomptarget constructor
+    for (auto *Desc : DelayedBinDesc) {
+      __tgt_register_lib(Desc);
+    }
+    DelayedBinDesc.clear();
+    RTLsLoaded = true;
----------------
Maetveis wrote:
> This will just add them to the delayed vector again.
ha, good spot! Broke that in the refactor to methods


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142249