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

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

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`.

> 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.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D64080