Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 9 months ago by rmcilroy Modified:
6 years, 9 months ago Reviewers:
Mark Mentovai, Nico, Rodolph Perfetta (ARM), JF CC:
chromium-reviews, erikwright+watch_chromium.org, (unused - use chromium), Ted C, Lei Zhang Base URL:
https://chromium.googlesource.com/chromium/src.git@a64_gyp Visibility:
Public. |
DescriptionAdd 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
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : Use MemoryBarrier() where appropriate #
Total comments: 4
Patch Set 4 : Change constraints to +Q #Patch Set 5 : Add todo comments. #Patch Set 6 : #
Created: 6 years, 9 months ago
Messages
Total messages: 20 (0 generated)
This is taken and adapted from the V8 atomicops. PTAL.
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.
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: 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.
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.
lgtm
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"?
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.
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?
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.
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.
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. ...
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.
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.
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/206373005/100001
Message was sent while issue was closed.
Change committed as 258985
Message was sent while issue was closed.
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
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
