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

Sat Oct 30 13:47:16 PDT 2021

tianshilei1992 added inline comments.

================
Comment at: openmp/libomptarget/DeviceRTL/src/Parallelism.cpp:129
+    // __kmpc_target_deinit may not hold.
+    synchronize::threadsAligned();
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112861