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

[Hans Wennborg via Openmp-commits]

On Wed, Jun 8, 2016 at 10:55 AM, Pawel Osmialowski
<Pawel.Osmialowski at arm.com> wrote:
> 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

I didn't find any use of std::atomic<> in kmp_os.h. I assume you mean
kmp_lock.h?

Anyway, this is a mess. With my change, building with GCC on Linux fails:

../projects/openmp/runtime/src/kmp_lock.cpp:1073:87: error: no
matching function for call to 'atomic_load_explicit(std::atomic_bool*,
std::memory_order)'
     return std::atomic_load_explicit( &lck->lk.initialized,
std::memory_order_relaxed ) && ( lck->lk.self == lck);

                ^
../projects/openmp/runtime/src/kmp_lock.cpp:1073:87: note: candidates are:
In file included from ../projects/openmp/runtime/src/kmp_lock.cpp:17:0:
/usr/include/c++/4.8/atomic:834:5: note: template<class _ITp> _ITp
std::atomic_load_explicit(const std::atomic<_ITp>*, std::memory_order)
     atomic_load_explicit(const atomic<_ITp>* __a, memory_order __m) noexcept
     ^
/usr/include/c++/4.8/atomic:834:5: note:   template argument
deduction/substitution failed:
../projects/openmp/runtime/src/kmp_lock.cpp:1073:87: note:
'std::atomic_bool' is not derived from 'const std::atomic<_ITp>'
     return std::atomic_load_explicit( &lck->lk.initialized,
std::memory_order_relaxed ) && ( lck->lk.self == lck);

                ^


I've gone ahead and committed r272271 which uses an #ifdef for MSVC.
It's not very nice, but at least it unbreaks the build.

Thanks,
Hans


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