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

[Pawel Osmialowski via Openmp-commits]

Would be great if I could compile it here first.

Cheers,
Paul

On Tue, 2016-06-07 at 09:07 -0700, Hans Wennborg 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.