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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Parallelism.cpp:129
+    // __kmpc_target_deinit may not hold.
+    synchronize::threadsAligned();
+
----------------
ye-luo wrote:
> 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?
OK. The destructor is only for a single value and having one alignment for multiple destructors is beneficial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112861