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

Tue Jul 2 14:20:36 PDT 2019

jdoerfert accepted this revision.
jdoerfert added a comment.

In D64080#1567088 <https://reviews.llvm.org/D64080#1567088>, @ABataev wrote:

> In D64080#1567062 <https://reviews.llvm.org/D64080#1567062>, @jdoerfert wrote:
>
> > This looks generally good to me and I think we should get this fix in. I have one nit inlined, and two follow up questions below:
> >
> > 1. Do we need to be interoperable with `std::thread`? I mean, would it be sufficient to ask for the OpenMP thread ID instead of the `std::this_thread::get_id()`?
>
>
> It would be good to use OpenMP thread id, but, unfortunately, OpenMP thread id is not passed to the offloading functions (tgt_target etc.). That was the reason to use `std::this_thread::get_id()` instead of OpenMP thread id.
>  We can call `__kmpc_global_thread_num(nullptr)` directly instead. libomptarget already depends on libomp to support async offloading, so, maybe, we can use this libomp function instead of `std::thread`.

Looks better now.

>> 2. Shouldn't we move the whole loop trip count logic into the target offload call? Afaik, we currently emit sth. like: ``` __kmpc_set_upcoming_trip_count(N); __kmpc_target(....); ``` Now if we would move the N as a parameter into the "target" call we would not need to store it anyway, or am I missing something here?
> 
> I thought about it and I think it is a good idea. But it is a completely different problem. We can't change the interface of the existing functions (for compatibility), instead, we need to provide the whole bunch of new functions with some new params. I just want to fix the problem with the existing kmpc_push_tripcount function. Redesign requires some additional work.

Fair point. Let's put that on the TODO list and talk about what else we want to include on the mailing list.

Good to commit.

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D64080