ARM64: Fix and improve atomic operations.

ARM64: Fix and improve atomic operations.

* The 'compare and swap' operations should enforce memory ordering even when
  the exchange does not occur.

* The exclusive monitor does not need to be cleared by CLREX if a LDRX was
  not followed by a matching STREX.

* Use LDAR and STLR where possible.

* Use the 'I' and 'J' constraints to hint for constants valid for immediate
  values.

R=jfb@chromium.org, rmcilroy@chromium.org, ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20446

[rmcilroy, 2014-04-01 10:41:19]

Generally looks good to me, one question and adding JFB since he had some
comments on the implementation in Chromium/Base.

https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
File src/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
src/atomicops_internals_arm64_gcc.h:57: : [old_value]"IJr" (old_value),
Do the I and J constraints require that the value fit in an immediate (12 bits)
or is it just a hint that an immediate should be used if it does fit?

[Alexandre Rames, 2014-04-01 10:50:55]

https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
File src/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
src/atomicops_internals_arm64_gcc.h:57: : [old_value]"IJr" (old_value),
On 2014/04/01 10:41:19, rmcilroy wrote:
> Do the I and J constraints require that the value fit in an immediate (12
bits)
> or is it just a hint that an immediate should be used if it does fit?

It is just a hint. If the value does not fit it will use a register. (Without
them it uses always uses a register.)

[jochen (gone - plz use gerrit), 2014-04-01 10:54:08]

On 2014/04/01 10:50:55, Alexandre Rames wrote:
>
https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
> File src/atomicops_internals_arm64_gcc.h (right):
> 
>
https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
> src/atomicops_internals_arm64_gcc.h:57: : [old_value]"IJr" (old_value),
> On 2014/04/01 10:41:19, rmcilroy wrote:
> > Do the I and J constraints require that the value fit in an immediate (12
> bits)
> > or is it just a hint that an immediate should be used if it does fit?
> 
> It is just a hint. If the value does not fit it will use a register. (Without
> them it uses always uses a register.)

will we need to update the implementation in chromium's src/base as well?

[rmcilroy, 2014-04-01 10:57:59]

On 2014/04/01 10:54:08, jochen wrote:
> On 2014/04/01 10:50:55, Alexandre Rames wrote:
> >
>
https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
> > File src/atomicops_internals_arm64_gcc.h (right):
> > 
> >
>
https://codereview.chromium.org/220793002/diff/1/src/atomicops_internals_arm6...
> > src/atomicops_internals_arm64_gcc.h:57: : [old_value]"IJr" (old_value),
> > On 2014/04/01 10:41:19, rmcilroy wrote:
> > > Do the I and J constraints require that the value fit in an immediate (12
> > bits)
> > > or is it just a hint that an immediate should be used if it does fit?
> > 
> > It is just a hint. If the value does not fit it will use a register.
(Without
> > them it uses always uses a register.)
> 
> will we need to update the implementation in chromium's src/base as well?

Yes (and also third_party/protobuf which has it's own copy).  I can do this once
this has been submitted to V8.

[JF, 2014-04-01 16:08:09]

I'm not sure I understand why "memory" is kept in some places and removed in
others.

lgtm otherwise.

[Alexandre Rames, 2014-04-01 16:19:36]

On 2014/04/01 16:08:09, JF wrote:
> I'm not sure I understand why "memory" is kept in some places and removed in
> others.
> 
> lgtm otherwise.

From http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html :

If your assembler instructions access memory in an unpredictable fashion, add
‘memory’ to the list of clobbered registers. This causes GCC to not keep memory
values cached in registers across the assembler instruction and not optimize
stores or loads to that memory. You also should add the volatile keyword if the
memory affected is not listed in the inputs or outputs of the asm, as the
‘memory’ clobber does not count as a side-effect of the asm.

So memory is needed when we have barriers, but not when we use 'simple'
load/store instructions.

[JF, 2014-04-01 16:28:59]

On 2014/04/01 16:19:36, Alexandre Rames wrote:
> On 2014/04/01 16:08:09, JF wrote:
> > I'm not sure I understand why "memory" is kept in some places and removed in
> > others.
> > 
> > lgtm otherwise.
> 
> From http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html :
> 
> If your assembler instructions access memory in an unpredictable fashion, add
> ‘memory’ to the list of clobbered registers. This causes GCC to not keep
memory
> values cached in registers across the assembler instruction and not optimize
> stores or loads to that memory. You also should add the volatile keyword if
the
> memory affected is not listed in the inputs or outputs of the asm, as the
> ‘memory’ clobber does not count as a side-effect of the asm.
> 
> So memory is needed when we have barriers, but not when we use 'simple'
> load/store instructions.

I see, you want to ensure no memory values are registerized across
acquire/release?

In that case I think "memory" should be kept in functions that have a single
barriers, and the barriers should probably be moved into the asm statements,
otherwise the compiler could technically move memory operations between a
barrier and the asm statement. e.g. Acquire_CompareAndSwap uses
NoBarrier_CompareAndSwap followed by MemoryBarrier, and nothing technically
prevents memory operations to move around.

[JF, 2014-04-01 16:40:25]

On 2014/04/01 16:28:59, JF wrote:
> On 2014/04/01 16:19:36, Alexandre Rames wrote:
> > On 2014/04/01 16:08:09, JF wrote:
> > > I'm not sure I understand why "memory" is kept in some places and removed
in
> > > others.
> > > 
> > > lgtm otherwise.
> > 
> > From http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html :
> > 
> > If your assembler instructions access memory in an unpredictable fashion,
add
> > ‘memory’ to the list of clobbered registers. This causes GCC to not keep
> memory
> > values cached in registers across the assembler instruction and not optimize
> > stores or loads to that memory. You also should add the volatile keyword if
> the
> > memory affected is not listed in the inputs or outputs of the asm, as the
> > ‘memory’ clobber does not count as a side-effect of the asm.
> > 
> > So memory is needed when we have barriers, but not when we use 'simple'
> > load/store instructions.
> 
> I see, you want to ensure no memory values are registerized across
> acquire/release?
> 
> In that case I think "memory" should be kept in functions that have a single
> barriers, and the barriers should probably be moved into the asm statements,
> otherwise the compiler could technically move memory operations between a
> barrier and the asm statement. e.g. Acquire_CompareAndSwap uses
> NoBarrier_CompareAndSwap followed by MemoryBarrier, and nothing technically
> prevents memory operations to move around.

Unless that's on purpose (i.e. you're telling the compiler to acquire or release
at the time it finds most convenient, taking the barrier into account), in which
case this should be documented.

[Alexandre Rames, 2014-04-01 17:34:15]

On 2014/04/01 16:40:25, JF wrote:
> On 2014/04/01 16:28:59, JF wrote:
> > On 2014/04/01 16:19:36, Alexandre Rames wrote:
> > > On 2014/04/01 16:08:09, JF wrote:
> > > > I'm not sure I understand why "memory" is kept in some places and
removed
> in
> > > > others.
> > > > 
> > > > lgtm otherwise.
> > > 
> > > From http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html :
> > > 
> > > If your assembler instructions access memory in an unpredictable fashion,
> add
> > > ‘memory’ to the list of clobbered registers. This causes GCC to not keep
> > memory
> > > values cached in registers across the assembler instruction and not
optimize
> > > stores or loads to that memory. You also should add the volatile keyword
if
> > the
> > > memory affected is not listed in the inputs or outputs of the asm, as the
> > > ‘memory’ clobber does not count as a side-effect of the asm.
> > > 
> > > So memory is needed when we have barriers, but not when we use 'simple'
> > > load/store instructions.
> > 
> > I see, you want to ensure no memory values are registerized across
> > acquire/release?
> > 
> > In that case I think "memory" should be kept in functions that have a single
> > barriers, and the barriers should probably be moved into the asm statements,
> > otherwise the compiler could technically move memory operations between a
> > barrier and the asm statement. e.g. Acquire_CompareAndSwap uses
> > NoBarrier_CompareAndSwap followed by MemoryBarrier, and nothing technically
> > prevents memory operations to move around.

Yes! Forgot about that. I'll do a test to see what the compiler generates in
both cases.

> 
> Unless that's on purpose (i.e. you're telling the compiler to acquire or
release
> at the time it finds most convenient, taking the barrier into account), in
which
> case this should be documented.

[Alexandre Rames, 2014-04-02 09:42:26]

Following JF's comment, I updated the clobber list of NoBarrier operations to
include "memory", and added a comment explaining why.

Tests showed no difference in the code generated between integrating the barrier
in the inline asm and using a function MemoryBarrier(), so I prefer not
duplicating the assembly code.

[rmcilroy, 2014-04-02 10:13:43]

On 2014/04/02 09:42:26, Alexandre Rames wrote:
> Following JF's comment, I updated the clobber list of NoBarrier operations to
> include "memory", and added a comment explaining why.
> 
> Tests showed no difference in the code generated between integrating the
barrier
> in the inline asm and using a function MemoryBarrier(), so I prefer not
> duplicating the assembly code.

lgtm, thanks for the explination.

[JF, 2014-04-02 16:03:49]

lgtm

[Alexandre Rames, 2014-04-02 16:29:41]

On 2014/04/02 16:03:49, JF wrote:
> lgtm

(Still need approval from an owner of the file (Ulan and Jochen for example) to
allow committing.)

[ulan, 2014-04-02 16:30:05]

On 2014/04/02 16:29:41, Alexandre Rames wrote:
> On 2014/04/02 16:03:49, JF wrote:
> > lgtm
> 
> (Still need approval from an owner of the file (Ulan and Jochen for example)
to
> allow committing.)

lgtm

[Alexandre Rames, 2014-04-02 16:37:42]

Committed patchset #2 manually as r20446 (presubmit successful).