RFR #2 (M) 8148146: Integrate new internal Unsafe entry points, and basic intrinsic support for VarHandles

[Andrew Dinn]

On 04/07/16 16:16, Andrew Haley wrote:
> [CORRECTED.  Please igfnore previous version.]
> 
> I'm writing the AArch64 intrinsics for the weak entry points, and
> the generated code for volatile compare&swap looks wrong to me.
> 
> I'm seeing this:
> 
>  	membar_release
>  	cmpxchg
>  	membar_acquire
> 
> ... but this is not enough for volatile.  It is enough for
> AcquireRelease, but for Volatile It needs a trailing membar_volatile
> to make the store visible to observers in other threads.  Looking at
> LibraryCallKit::inline_unsafe_access, it seems to me that the same
> code is generated for both Release and Volatile:
> 
>   // Add the trailing membar surrounding the access
>   insert_mem_bar(Op_MemBarCPUOrder);
> 
>   switch (access_kind) {
>     case Relaxed:
>     case Release:
>       break; // do nothing
>     case Acquire:
>     case Volatile:
>       insert_mem_bar(Op_MemBarAcquire);
>       // !support_IRIW_for_not_multiple_copy_atomic_cpu handled in platform code
>       break;
>     default:
>       ShouldNotReachHere();
>   }
> 
> But it really shouldn't be: a volatile store should be followed by
> MemBarVolatile, like so:
> 
>   switch (access_kind) {
>     case Relaxed:
>     case Release:
>       break; // do nothing
>     case Acquire:
>       insert_mem_bar(Op_MemBarAcquire);
>       break;
>     case Volatile:
>       insert_mem_bar(Op_MemBarVolatile);
>       break;
>     default:
>       ShouldNotReachHere();
>   }
> 
> ... and this does indeed generate the correct code.

Hmm, logically I think you are correct that in terms of what the barrier
nodes are /meant/ to represent there should be a MemBarVolatile in the
sequence. However, the code Alexei wrote was written to follow the
existing pattern of usage which was established to avoid the double use
of a locked instruction that would result on x86.

So, when we have a CompareAndSwapX operation the AArch64 back end has
always expected to see

  MembarRelease
  ConpareAndSwapX
  MembarAcquire

well, that or the variant

  MembarRelease
  MembarCPUOrder
  ConpareAndSwapX
  MembarCPUOrder
  MembarAcquire

Originally AArch64 handled the need to order stores /following/ the
exchange store by naively translating the above to

  dmb (LoadStore|StoreStore)
  ...
  ldrx
  ...
  stlrx
  ..
  dmb (LoadLoad|LoadStore)

Of course with that translation the initial store barrier is redundant.
So, we then added optimising rules which spot this pattern and improve
the generated code. They inhibit the translation of the
MembarRelease/Acquire and plant a ldarx stlrx pair


  ...
  ldarx
  ...
  stlrx
  ..

which does all we need. If you are not seeing an stlrx then we have a
problem. Is that the case?

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander