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

[Hans Wennborg via Openmp-commits]

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