[Openmp-commits] [PATCH] D64080: [OPENMP]Make __kmpc_push_tripcount thread safe.

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel accepted this revision.
hfinkel added a comment.

In D64080#1569501 <https://reviews.llvm.org/D64080#1569501>, @hfinkel wrote:

> In D64080#1569375 <https://reviews.llvm.org/D64080#1569375>, @ABataev wrote:
>
> > In D64080#1569363 <https://reviews.llvm.org/D64080#1569363>, @hfinkel wrote:
> >
> > > In D64080#1569334 <https://reviews.llvm.org/D64080#1569334>, @ABataev wrote:
> > >
> > > > In D64080#1569319 <https://reviews.llvm.org/D64080#1569319>, @hfinkel wrote:
> > > >
> > > > > In D64080#1569299 <https://reviews.llvm.org/D64080#1569299>, @ABataev wrote:
> > > > >
> > > > > > In D64080#1569266 <https://reviews.llvm.org/D64080#1569266>, @hfinkel wrote:
> > > > > >
> > > > > > > In D64080#1568348 <https://reviews.llvm.org/D64080#1568348>, @ABataev wrote:
> > > > > > >
> > > > > > > > Added note.
> > > > > > >
> > > > > > >
> > > > > > > The patch description says that you add a trip count "per thread", but the code is adding a trip count per device. Could this still cause a problem if multiple threads have target regions for the same device?
> > > > > >
> > > > > >
> > > > > > No. We already have tripcount per device: each device stores its unique tripcount value, but only one. I extended the existing solution to have the tripcount not only per device, but also per host thread.
> > > > >
> > > > >
> > > > > Would there be any advantage to using `__kmpc_threadprivate_register` and `__kmpc_threadprivate` (or similar) instead of storing a map with thread ids?
> > > >
> > > >
> > > > Hard to tell, but definitely ee increase dependency between libomptarget and libomp. and,  seems to me, threadprivate interface also uses some kind of hash table. We can use unordered_map instead.
> > >
> > >
> > > Given that we expect this to be temporary, I'm okay with it either way. unordered_map is probably a little bit better than std::map. I expect the number of entries in the map to be small because __kmpc_global_thread_num returns OpenMP thread ids, not system TIDs, right?
> >
> >
> > Yes.
>
>
> SGTM.


A note on naming, but this also LGTM.

I chatted offline with Johannes, and I agree with him, that longer term we should have a new interface that doesn't require this "push" precall. Regardless, I think that the comment is fine, in that, so long as we have to support this interface (which we might need for backward compatibility even after we add a new interface), it's fine to note where a cleaner location for that state data might be.



================
Comment at: libomptarget/src/device.h:101
+  // moved into the target task in libomp.
+  std::map<int32_t, uint64_t> loopTripCnt;
 
----------------
While we're changing this, we should also update it to following our naming conventions:

  loopTripCnt -> LoopTripCnt


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D64080