[Openmp-commits] [openmp] r271324 - Use C++11 atomics for ticket locks implementation

[Pawel Osmialowski via Openmp-commits]

Hi Hans,

I did following changes locally:
- kmp_os.h: std::atomic<bool> -> std::atomic_bool, std::atomic<unsigned>
-> std::atomic_uint, std::atomic<int> -> std::atomic_int
- kmp_lock.cpp: (std::atomic<unsigned> *) -> (std::atomic_uint *)

Šand it compiles and runs fine on my machines. If you have ability to
commit, feel free to publish your change.

Best regards,
________________________________________


Paul Osmialowski
Staff Engineer, ARM Manchester Design Centre






On 07/06/2016 17:07, "hwennborg at google.com on behalf of Hans Wennborg"
<hwennborg at google.com on behalf of hans at chromium.org> wrote:

>This broke compiling with Visual Studio 2013, see below.
>
>On Tue, May 31, 2016 at 1:20 PM, Paul Osmialowski via Openmp-commits
><openmp-commits at lists.llvm.org> wrote:
>> Author: pawosm01
>> Date: Tue May 31 15:20:32 2016
>> New Revision: 271324
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=271324&view=rev
>> Log:
>> Use C++11 atomics for ticket locks implementation
>>
>> This patch replaces use of compiler builtin atomics with
>> C++11 atomics for ticket locks implementation. Ticket locks
>> are used in critical places of the runtime, e.g. in the tasking
>> mechanism.
>>
>> The main reason this change was introduced is the problem
>> with work stealing function on ARM architecture which suffered
>> from nasty race condition. It turned out that the root cause of
>> the problem lies in the way ticket locks are implemented. Changing
>> compiler builtins into C++11 atomics solves the problem.
>>
>> Two assertions were added into kmp_tasking.c which are useful
>> for detecting early symptoms of something wrong going on with
>> work stealing, which were among the possible outcomes of the
>> race condition.
>>
>> Differential Revision: http://reviews.llvm.org/D19878
>
>[..]
>
>>  struct kmp_base_ticket_lock {
>>      // `initialized' must be the first entry in the lock data
>>structure!
>> -    volatile union kmp_ticket_lock * initialized;  // points to the
>>lock union if in initialized state
>> -    ident_t const *     location;     // Source code location of
>>omp_init_lock().
>> -    volatile kmp_uint32 next_ticket;  // ticket number to give to next
>>thread which acquires
>> -    volatile kmp_uint32 now_serving;  // ticket number for thread
>>which holds the lock
>> -    volatile kmp_int32  owner_id;     // (gtid+1) of owning thread, 0
>>if unlocked
>> -    kmp_int32           depth_locked; // depth locked, for nested
>>locks only
>> -    kmp_lock_flags_t    flags;        // lock specifics, e.g. critical
>>section lock
>> +    std::atomic<bool>     initialized;
>> +    volatile union kmp_ticket_lock *self; // points to the lock union
>> +    ident_t const *       location;       // Source code location of
>>omp_init_lock().
>> +    std::atomic<unsigned> next_ticket;    // ticket number to give to
>>next thread which acquires
>> +    std::atomic<unsigned> now_serving;    // ticket number for thread
>>which holds the lock
>> +    std::atomic<int>      owner_id;       // (gtid+1) of owning
>>thread, 0 if unlocked
>> +    std::atomic<int>      depth_locked;   // depth locked, for nested
>>locks only
>> +    kmp_lock_flags_t      flags;          // lock specifics, e.g.
>>critical section lock
>
>d:\src\llvm_package_271940\llvm\projects\openmp\runtime\src\kmp_lock.h(261
>) : er
>ror C2621: 'kmp_ticket_lock::lk' : illegal union member; type
>'kmp_base_ticket_l
>ock' has a copy constructor
>
>It seems at least one of these std::atomic<> types have a non-trivial
>copy-constructor.
>
>One way to fix this seems to be to use std::atomic_{bool,uint,int}
>instead, as those types don't seem to have that problem. Shall I go
>ahead and commit a patch for that?
>
>Thanks,
>Hans
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.