Add Arm64 atomicops to base.

Add Arm64 atomicops to base.

These are taken from the implementation in v8/src/atomicops_internals_a64_gcc.h

BUG=354405
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258985

[rmcilroy, 2014-03-20 16:40:00]

This is taken and adapted from the V8 atomicops.  PTAL.

[Mark Mentovai, 2014-03-20 16:52:21]

LGTM for base OWNERS review, but I haven’t closely scrutinized the actual ARM
assembler code because I assume another reviewer will be more competent to
comment on it than I am.

[Nico, 2014-03-20 17:35:01]

This is the first time I'm looking at arm64 code. Most of it makes sense to me,
but that doesn't mean much :-) But if it's from v8, it'll likely mostly work.

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
File base/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
base/atomicops_internals_arm64_gcc.h:19: "dmb ish                               
  \n\t"  // Data memory barrier.
Rodolph: After looking through docs a bit, it sounds like "ish" roughly means
"barrier for all cpu cores", while "osh" would include things like gpus – is
that roughly correct?

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
base/atomicops_internals_arm64_gcc.h:109: : "memory"
This looks identical to the

  MemoryBarrier();
  Atomic32 result = NoBarrier_AtomicIncrement(ptr, increment);
  MemoryBarrier();

that the 32 bit arm code does. Any reason to not just use that?

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
base/atomicops_internals_arm64_gcc.h:181: );  // NOLINT
Also here and in the functions below: the arm32 version just calls
MemoryBarrier() – it looks like this could happen here (and below) too?

[Rodolph Perfetta, 2014-03-20 18:27:34]

> Rodolph: After looking through docs a bit, it sounds like "ish" roughly means
> "barrier for all cpu cores", while "osh" would include things like gpus
– is
> that roughly correct?

Yes.

I'll do the review tomorrow.

[rmcilroy, 2014-03-20 18:38:42]

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
File base/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
base/atomicops_internals_arm64_gcc.h:109: : "memory"
On 2014/03/20 17:35:01, Nico (on GMT time Mar 15 - 24) wrote:
> This looks identical to the
> 
>   MemoryBarrier();
>   Atomic32 result = NoBarrier_AtomicIncrement(ptr, increment);
>   MemoryBarrier();
> 
> that the 32 bit arm code does. Any reason to not just use that?

Good point - done.

https://codereview.chromium.org/206373005/diff/1/base/atomicops_internals_arm...
base/atomicops_internals_arm64_gcc.h:181: );  // NOLINT
On 2014/03/20 17:35:01, Nico (on GMT time Mar 15 - 24) wrote:
> Also here and in the functions below: the arm32 version just calls
> MemoryBarrier() – it looks like this could happen here (and below) too?

Done.

[Nico, 2014-03-20 18:42:09]

lgtm

[JF, 2014-03-20 21:55:50]

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
File base/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
base/atomicops_internals_arm64_gcc.h:42: : [ptr]"r" (ptr),
Shouldn't ptr always be "m"?

[Rodolph Perfetta (ARM), 2014-03-21 15:04:45]

On 2014/03/20 16:40:00, rmcilroy wrote:
> This is taken and adapted from the V8 atomicops.  PTAL.

Overall looks fine to me.

We are planning to publish a slightly tweaked version of this file for V8 next
week, might be worth propagating the changes then.

[rmcilroy, 2014-03-21 15:27:19]

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
File base/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
base/atomicops_internals_arm64_gcc.h:42: : [ptr]"r" (ptr),
On 2014/03/20 21:55:50, JF wrote:
> Shouldn't ptr always be "m"?

I'm not entirely sure on this (I'm not very knowledgeable on gcc asm semantics).
 It doesn't seem to be "m" on atomicops_internal_arm_gcc.h from what I can tell
(there does seem to be an "+m"(*ptr), but that seems unused to me).

This is what is currently in V8.  Rodolph: do you have any advice on "r" vs "m"
here?

[JF, 2014-03-21 16:08:51]

I'm also wondering why the acquire/release versions of the exclusive load/store
instructions aren't used? I'm not sure if the semantics are the same between ARM
and the implied API that Chrome's "acquire" and "release" have, but it seems
like we could do away with the barriers.

I'm not sure, is CBNZ supposed to mark the condition flags as dirty? I don't
think so looking at DDI0487A ARMv8 ARM C5.6.30, but my A64 is rusty.

[Rodolph Perfetta (ARM), 2014-03-21 17:27:53]

On 2014/03/21 16:08:51, JF wrote:
> I'm also wondering why the acquire/release versions of the exclusive
load/store
> instructions aren't used? I'm not sure if the semantics are the same between
ARM
> and the implied API that Chrome's "acquire" and "release" have, but it seems
> like we could do away with the barriers.
> 
> I'm not sure, is CBNZ supposed to mark the condition flags as dirty? I don't
> think so looking at DDI0487A ARMv8 ARM C5.6.30, but my A64 is rusty.

cbnz does not touch the flags.

[JF, 2014-03-21 17:57:37]

Below, and update on inline asm memory references.

Roland is further asking why the GCC __sync_* intrinsics aren't used instead
since they were available at the same time as the A64 assembler? He suggests
adding a comment that explains with they aren't used.

Rodolf clarified my question on CBNZ, so that's done.

There's still the question of acquire/release usage.

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
File base/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
base/atomicops_internals_arm64_gcc.h:42: : [ptr]"r" (ptr),
On 2014/03/21 15:27:20, rmcilroy wrote:
> On 2014/03/20 21:55:50, JF wrote:
> > Shouldn't ptr always be "m"?
> 
> I'm not entirely sure on this (I'm not very knowledgeable on gcc asm
semantics).
>  It doesn't seem to be "m" on atomicops_internal_arm_gcc.h from what I can
tell
> (there does seem to be an "+m"(*ptr), but that seems unused to me).
> 
> This is what is currently in V8.  Rodolph: do you have any advice on "r" vs
"m"
> here?

I looked into this and checked with Roland McGrath, "+m" would be correct if an
immediate displacement were allowed, but for these instructions it isn't so "+Q"
is actually the right choice.

It should therefore be:

"ldxr %w[prev], %[ptr]" // Note the brackets are gone.
...
: // ...
: [ptr]"+Q" (*ptr), // Note the dereference.
...

[rmcilroy, 2014-03-24 15:19:24]

I'd like to land this to at least get Arm64 building - could we have the
acquire/release discussion offline and do that in a different CL if it requires
any changes?

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
File base/atomicops_internals_arm64_gcc.h (right):

https://codereview.chromium.org/206373005/diff/40001/base/atomicops_internals...
base/atomicops_internals_arm64_gcc.h:42: : [ptr]"r" (ptr),
On 2014/03/21 17:57:38, JF wrote:
> On 2014/03/21 15:27:20, rmcilroy wrote:
> > On 2014/03/20 21:55:50, JF wrote:
> > > Shouldn't ptr always be "m"?
> > 
> > I'm not entirely sure on this (I'm not very knowledgeable on gcc asm
> semantics).
> >  It doesn't seem to be "m" on atomicops_internal_arm_gcc.h from what I can
> tell
> > (there does seem to be an "+m"(*ptr), but that seems unused to me).
> > 
> > This is what is currently in V8.  Rodolph: do you have any advice on "r" vs
> "m"
> > here?
> 
> I looked into this and checked with Roland McGrath, "+m" would be correct if
an
> immediate displacement were allowed, but for these instructions it isn't so
"+Q"
> is actually the right choice.
> 
> It should therefore be:
> 
> "ldxr %w[prev], %[ptr]" // Note the brackets are gone.
> ...
> : // ...
> : [ptr]"+Q" (*ptr), // Note the dereference.
> ...

Done.

[JF, 2014-03-24 15:54:04]

On 2014/03/24 15:19:24, rmcilroy wrote:
> I'd like to land this to at least get Arm64 building - could we have the
> acquire/release discussion offline and do that in a different CL if it
requires
> any changes?

Works for me, acquire/release would indeed be a nice-to-have but isn't required.

Could we also add the comment on why __sync_* isn't used in a later CL?

Otherwise LGTM.

[rmcilroy, 2014-03-24 16:28:04]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2014-03-24 16:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/206373005/100001

[commit-bot: I haz the power, 2014-03-24 18:52:40]

Change committed as 258985

[Alexandre Rames, 2014-03-25 17:06:43]

On 2014/03/24 18:52:40, I haz the power (commit-bot) wrote:
> Change committed as 258985


I have reworked these for v8. The patch should be uploaded for review soon.
There are fixes for the CompareAndSwap which should perform a barrier even when
the swap does not occur.
There are also improvements like improved constraints (including the "+Q", but
also using "I"), removed non-required clrex, etc.

Alexandre

[rmcilroy, 2014-03-25 17:11:50]

On 2014/03/25 17:06:43, Alexandre Rames wrote:
> On 2014/03/24 18:52:40, I haz the power (commit-bot) wrote:
> > Change committed as 258985
> 
> 
> I have reworked these for v8. The patch should be uploaded for review soon.
> There are fixes for the CompareAndSwap which should perform a barrier even
when
> the swap does not occur.
> There are also improvements like improved constraints (including the "+Q", but
> also using "I"), removed non-required clrex, etc.
> 
> Alexandre

Thanks Alexandre.  Did you look into the use of acquire / release exclusive load
/ stores as discussed up by JF?

[Alexandre Rames, 2014-03-25 17:35:29]

On 2014/03/25 17:11:50, rmcilroy wrote:
> On 2014/03/25 17:06:43, Alexandre Rames wrote:
> > On 2014/03/24 18:52:40, I haz the power (commit-bot) wrote:
> > > Change committed as 258985
> > 
> > 
> > I have reworked these for v8. The patch should be uploaded for review soon.
> > There are fixes for the CompareAndSwap which should perform a barrier even
> when
> > the swap does not occur.
> > There are also improvements like improved constraints (including the "+Q",
but
> > also using "I"), removed non-required clrex, etc.
> > 
> > Alexandre
> 
> Thanks Alexandre.  Did you look into the use of acquire / release exclusive
load
> / stores as discussed up by JF?

I haven't done this one yet, but plan to do so in this patch.