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

Narayanaswamy, Ravi via Openmp-commits openmp-commits at lists.llvm.org
Mon Jul 8 15:47:10 PDT 2019


> Ravi, as far as I can tell, the only thing that was added was the interface with the device_id, but the device_id is not yet used for anything?
Correct, it is not yet used for anything.  IBM may be using it to track in their source base to track dependences between devices and offload them without hosting managing the dependences.
Ravi

-----Original Message-----
From: Finkel, Hal J. [mailto:hfinkel at anl.gov] 
Sent: Wednesday, July 3, 2019 2:57 PM
To: Alexey Bataev <a.bataev at outlook.com>; Narayanaswamy, Ravi <ravi.narayanaswamy at intel.com>; reviews+D64080+public+14328c31ffb14ae3 at reviews.llvm.org; Alexey Bataev <a.bataev at hotmail.com>; george_rokos at hotmail.com; Doerfert, Johannes <jdoerfert at anl.gov>; alexe at us.ibm.com
Cc: Churbanov, Andrey <Andrey.Churbanov at intel.com>; zhang.guansong at gmail.com; jfbastien at apple.com; openmp-commits at lists.llvm.org; kkwli0 at gmail.com; caomhin at us.ibm.com; deachempat at cray.com; ron.lieberman at amd.com; jatin.bhateja at gmail.com; lildmh at gmail.com; Gregory.Rodgers at amd.com; acjacob at us.ibm.com; jlebar at google.com
Subject: Re: [PATCH] D64080: [OPENMP]Make __kmpc_push_tripcount thread safe.


On 7/3/19 10:40 AM, Alexey Bataev wrote:
>
>
> -------------
> Best regards,
> Alexey Bataev
> 03.07.2019 11:33, Narayanaswamy, Ravi пишет:
>>> To put it into target task allocated memory is completely different problem, which requires some redesign and full support of target tasks.
>> We do have a target task in libomp.  We recently added the device number to this target task for async support.
>> Ravi
>
> Yes, but it does not have any target specific functionality in it, it 
> just calls the regular task alloc function. As I understand, there 
> should be some additional functionality for target tasks.
>
> What I want to do in this patch, I just want to fix the bug in the 
> existing solution. I don't know how much we'll have to live with this, 
> I think it is good to fix the bugs in the existing solution even if we 
> plan to have a new one.
>

This certainly makes sense if there's a non-trivial amount of work to be done in libomp in order to move the state there.

Ravi, as far as I can tell, the only thing that was added was the interface with the device_id, but the device_id is not yet used for anything?
C

  -Hal


>> -----Original Message-----
>> From: Alexey Bataev via Phabricator [mailto:reviews at reviews.llvm.org]
>> Sent: Tuesday, July 2, 2019 5:48 PM
>> To:a.bataev at hotmail.com;george_rokos at hotmail.com;jdoerfert at anl.gov;al
>> exe at us.ibm.com Cc:hfinkel at anl.gov; Churbanov, 
>> Andrey<Andrey.Churbanov at intel.com>;zhang.guansong at gmail.com;jfbastien
>> @apple.com;openmp-commits at lists.llvm.org;kkwli0 at gmail.com;caomhin at us.
>> ibm.com;deachempat at cray.com;ron.lieberman at amd.com;jatin.bhateja at gmail
>> .com;lildmh at gmail.com;Gregory.Rodgers at amd.com;acjacob at us.ibm.com; 
>> Narayanaswamy, Ravi<ravi.narayanaswamy at intel.com>;jlebar at google.com
>> Subject: [PATCH] D64080: [OPENMP]Make __kmpc_push_tripcount thread safe.
>>
>> ABataev added a comment.
>>
>> In D64080#1567533<https://reviews.llvm.org/D64080#1567533>, @hfinkel wrote:
>>
>>> In D64080#1567348<https://reviews.llvm.org/D64080#1567348>, @ABataev wrote:
>>>
>>>> In D64080#1567327<https://reviews.llvm.org/D64080#1567327>, @grokos wrote:
>>>>
>>>>> A long time ago we had identified the problem with the loop trip count and if I recall correctly the proposed solution was to store the trip count per OpenMP task. The fix would be implemented in libomp because libomptarget has no notion of tasks.
>>>>>
>>>>> @AlexEichenberger Can you confirm whether or not this is the case? I may confuse this with something else...
>>>> The patch does exactly what you said: stores tripcount per task, but in the libomptarget, not libomp. When it will be implemented in libomp, we could remove this code. But we need fully functional implementation now, not in the future.
>>> If this should really be fixed in libomp, then we should fix it there. At the very least, there should be a FIXME about removing the workaround once the libomp fix is in place. @AndreyChurbanov, any thoughts on this?
>> Hal, currently target tripcount has nothing to do with libomp. It is stored as part of the device in libomptarget. So, I think, to fix this problem in libomptarget is fine.
>> To put it into target task allocated memory is completely different problem, which requires some redesign and full support of target tasks. Plus, just like I said, it is not true that libomptarget is thread agnostic. Actually, it is not, since it uses `__kmpc_omp_taskwait(NULL, 0)` for dependendent target regions. Moreover, it is not thread safe too, because uses not real thread ID,  but hardcoded `0` thread ID. It may lead to problems even in a single-threaded environment if the caller thread is not a master thread.
>>
>>
>> Repository:
>>    rOMP OpenMP
>>
>> CHANGES SINCE LAST ACTION
>>    https://reviews.llvm.org/D64080/new/
>>
>> https://reviews.llvm.org/D64080
>>
>>
>>
--
Hal Finkel
Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory



More information about the Openmp-commits mailing list