[Openmp-commits] [PATCH] D132676: [OpenMP][Fix] Track all threads that may delete an entry

[Guilherme Valarini via Phabricator via Openmp-commits]

gValarini added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:760
+      FromMapperBase = HstPtrBegin;
+    }
 
----------------
jdoerfert wrote:
> What's this block about?
To be completely honest, I cannot recall. Most of the checks that can affect `DelEntry` were transferred from `targetDataEnd`. Although I understand the other ones related to struct and ptr/obj mapping, I don´t get why the first element of a mapper-based entry should not have its reference count updated or should not be removed.

I know this was implemented in https://reviews.llvm.org/D100673, do you remember the purpose of this check? We could add a comment here explaining it better.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:772
+    const bool IsLastUser = TPR.Entry->decDataEndThreadCount() != 0;
+    if (DelEntry && (TPR.Entry->getTotalRefCount() != 0 || IsLastUser)) {
+      // The thread is not in charge of deletion anymore. Give up access
----------------
jdoerfert wrote:
> IsNotLastUser, right?
Ops, `IsNotLastUser` is the correct one. Thanks!


================
Comment at: openmp/libomptarget/src/omptarget.cpp:687
+    DelEntry = false;
+  }
+
----------------
ye-luo wrote:
> ye-luo wrote:
> > jdoerfert wrote:
> > > I think, this can leave an entry intact on the device:
> > > ```
> > > T1: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry, HDTTMap is released.
> > > T2: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry. HDTTMap is released.
> > > Both exit this function, TPRs are destroyed, DeleterThreadCount is decremented twice and reaches 1, but nobody executing the code that would delete the entry.
> > > ```
> > Can this be protected by not calling HDTTMap.destroy()?
> Second thought on this. If the mutex protection from HDTTMap is anyway needed, std::atomic seems likely not needed.
> Probably need to revisit D123443, D123444 and D123446.
@ye-luo we have a new version of this solution, I think your previous comments are now fixed.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:687
+    DelEntry = false;
+  }
+
----------------
gValarini wrote:
> ye-luo wrote:
> > ye-luo wrote:
> > > jdoerfert wrote:
> > > > I think, this can leave an entry intact on the device:
> > > > ```
> > > > T1: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry, HDTTMap is released.
> > > > T2: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry. HDTTMap is released.
> > > > Both exit this function, TPRs are destroyed, DeleterThreadCount is decremented twice and reaches 1, but nobody executing the code that would delete the entry.
> > > > ```
> > > Can this be protected by not calling HDTTMap.destroy()?
> > Second thought on this. If the mutex protection from HDTTMap is anyway needed, std::atomic seems likely not needed.
> > Probably need to revisit D123443, D123444 and D123446.
> @ye-luo we have a new version of this solution, I think your previous comments are now fixed.
I believe this does not happen anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132676