[Openmp-commits] [PATCH] D112861: [OpenMP][DeviceRTL] Fixed an issue that causes hang in SU3

Sat Oct 30 17:41:42 PDT 2021

ye-luo added inline comments.

================
Comment at: openmp/libomptarget/DeviceRTL/src/Parallelism.cpp:129
+    // __kmpc_target_deinit may not hold.
+    synchronize::threadsAligned();
+
----------------
tianshilei1992 wrote:
> ye-luo wrote:
> > ye-luo wrote:
> > > tianshilei1992 wrote:
> > > > ye-luo wrote:
> > > > > This explanation seems plausible. Are the synchronization requirement of ASSERT and why being documented somewhere?
> > > > Have no idea.
> > > synchronize::threadsAligned() has been called at the end of above scope.
> > > What caused the out of  sync and needs this additional call?
> > > Why ASSERT requires synchronization.
> > > All needs explanation or documentation.
> > > "Synchronize all threads to make sure every thread exits the scope above;"
> > > Just sounds plausible
> > Making code slower by the addtional synchronization can also potentially skip triggering the hang. So it needs to be well understood and explained.
> The key point here is the scope because state configuration is via RAII. The synchronization at the end of the scope can only make sure threads are aligned *before* exiting the scope, but since then, it is undetermined which threads exits first.
If the destructor sets a state which is supposed to be shared by all the threads, can synchronize::threadsAligned() be called inside the destructor?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112861