RFR(xs): 8211387: [Zero] atomic_copy64: Use ldrexd for atomic reads on ARMv7

[Severin Gehwolf]

Hi!

On Sun, 2018-10-07 at 07:52 +1000, David Holmes wrote:
> On 7/10/2018 6:11 AM, dean.long at oracle.com wrote:
> > On 10/5/18 5:18 PM, David Holmes wrote:
> > > On 6/10/2018 12:18 AM, Andrew Haley wrote:
> > > > On 10/05/2018 02:40 PM, Severin Gehwolf wrote:
> > > > > > Also I thought that if ldrex was not paired with a strex then you 
> > > > > > should
> > > > > > at least issue clrex - as done in generate_atomic_load_long() in
> > > > > > stubGenerator_arm.cpp
> > > > > 
> > > > > I'll defer to Andrew Haley for this.
> > > > 
> > > > I'm skeptical that CLREX is necessary. The classic CAS for Arm is
> > > > 
> > > >      MOV  R1, #0xFF             ; load the ?lock taken? value
> > > > try LDREX    R0, [LockAddr]    ; load the lock value
> > > >      CMP R0,  #0                ; is the lock free?
> > > >      STREXEQ  R1, R0, [LockAddr]; try and claim the lock
> > > >      CMPEQ R0, #0               ; did this succeed?
> > > >      BNE try                    ; no ? try again. . .
> > > > ; yes ? we have the lock
> > > > 
> > > > There's no CLREX, and no-one ever reported any problem. CLREX could be
> > > > a performance boost because it takes the cache line out of exclusive
> > > > state back to shared state, reducing later bus traffic.
> > > 
> > > I would see CLREX as a potential performance boost not something 
> > > needed for correctness. But you're right that failure paths from 
> > > typical CAS code won't execute the strex and don't include a clrex.
> > > 
> > 
> > I think it's needed for correctness only at context-switch time.  If you 
> > context switch after the LDREX, you don't want the exclusive state to 
> > remain for the next thread scheduled.
> 
> See also:
> 
> https://community.arm.com/processors/f/discussions/6572/synchronization-primitives-do-i-need-clrex
> 
> Seems if CLREX is needed for correctness it is at the OS level.
> 
> On performance concern see:
> 
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150921/300951.html
> 
> Bottom line seems to be:
> - clrex is not essential
> - clrex is probably not harmful to performance
> - clrex may benefit performance

OK. David, how would you like me to update the webrev then?

2a)
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8211387/webrev.02_a/

2b)
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8211387/webrev.02_b/

FWIW, the performance gain for Zero is probably negligible, but it
probably won't hurt either...

Thanks,
Severin