[Atomics] Make Atomics.exchange a builtin using TF

[Atomics] Make Atomics.exchange a builtin using TF

BUG=v8:4614
R=binji@chromium.org

Review-Url: https://codereview.chromium.org/2623633003
Cr-Commit-Position: refs/heads/master@{#43623}
Committed: https://chromium.googlesource.com/v8/v8/+/301c12376e5c675c5a3d5265d1bf2b3ab1e13679

[aseemgarg, 2017-01-09 23:36:54]

Ben, please take a look if it generally looks good. I'll add other architectures
in a day or so.

[binji, 2017-01-13 00:08:50]

looks good so far; some quick notes.

https://codereview.chromium.org/2623633003/diff/1/src/builtins/builtins-share...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/builtins/builtins-share...
src/builtins/builtins-sharedarraybuffer.cc:302: a.Bind(&u8);
nit: I'd prefer matching the order of the case labels: i8, u8, i16, u16, i32,
u32

https://codereview.chromium.org/2623633003/diff/1/src/compiler/machine-graph-...
File src/compiler/machine-graph-verifier.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/compiler/machine-graph-...
src/compiler/machine-graph-verifier.cc:456: case IrOpcode::kAtomicExchange:
Looks like kStore above is identical, can it be rolled in also?

https://codereview.chromium.org/2623633003/diff/1/src/compiler/x64/instructio...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/compiler/x64/instructio...
src/compiler/x64/instruction-selector-x64.cc:2401: if (type ==
MachineType::Uint8() || type == MachineType::Int8()) {
Looks like it is more common to do a switch on the MachineRepresentation.

[aseemgarg, 2017-01-14 01:48:19]

https://codereview.chromium.org/2623633003/diff/1/src/builtins/builtins-share...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/builtins/builtins-share...
src/builtins/builtins-sharedarraybuffer.cc:302: a.Bind(&u8);
On 2017/01/13 00:08:50, binji wrote:
> nit: I'd prefer matching the order of the case labels: i8, u8, i16, u16, i32,
> u32

Done.

https://codereview.chromium.org/2623633003/diff/1/src/compiler/machine-graph-...
File src/compiler/machine-graph-verifier.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/compiler/machine-graph-...
src/compiler/machine-graph-verifier.cc:456: case IrOpcode::kAtomicExchange:
On 2017/01/13 00:08:50, binji wrote:
> Looks like kStore above is identical, can it be rolled in also?

Done.

https://codereview.chromium.org/2623633003/diff/1/src/compiler/x64/instructio...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/compiler/x64/instructio...
src/compiler/x64/instruction-selector-x64.cc:2401: if (type ==
MachineType::Uint8() || type == MachineType::Int8()) {
On 2017/01/13 00:08:50, binji wrote:
> Looks like it is more common to do a switch on the MachineRepresentation.

Changed a little. We can't use MachineRepresentation now as signed and unsigned
have different behaviors.

[binji, 2017-01-17 21:45:50]

https://codereview.chromium.org/2623633003/diff/1/src/compiler/x64/instructio...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/1/src/compiler/x64/instructio...
src/compiler/x64/instruction-selector-x64.cc:2401: if (type ==
MachineType::Uint8() || type == MachineType::Int8()) {
On 2017/01/14 01:48:19, aseemgarg wrote:
> On 2017/01/13 00:08:50, binji wrote:
> > Looks like it is more common to do a switch on the MachineRepresentation.
> 
> Changed a little. We can't use MachineRepresentation now as signed and
unsigned
> have different behaviors.

Take a look at GetLoadOpcode/VisitCheckedLoad/VisitChangeInt32ToInt64, etc., it
looks like it is common to use a switch on MachineRepresentation, then check
rep.IsSigned().

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2220: __ xchgb(r, operand, true);
just add the appropriate extension instruction here, rather than adding a param
to xchgb

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/instru...
File src/compiler/x64/instruction-scheduler-x64.cc (left):

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/instru...
src/compiler/x64/instruction-scheduler-x64.cc:184: return kIsLoadOperation |
kHasSideEffect;
Where did this move to?

https://codereview.chromium.org/2623633003/diff/40001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/40001/src/x64/assembler-x64.c...
src/x64/assembler-x64.cc:1992: if (is_signed) {
This is the wrong place to put this code; the assembler is meant to map
more-or-less directly to instructions.

[aseemgarg, 2017-01-21 08:40:37]

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:2220: __ xchgb(r, operand, true);
On 2017/01/17 21:45:50, binji wrote:
> just add the appropriate extension instruction here, rather than adding a
param
> to xchgb

Done.

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/instru...
File src/compiler/x64/instruction-scheduler-x64.cc (left):

https://codereview.chromium.org/2623633003/diff/40001/src/compiler/x64/instru...
src/compiler/x64/instruction-scheduler-x64.cc:184: return kIsLoadOperation |
kHasSideEffect;
On 2017/01/17 21:45:50, binji wrote:
> Where did this move to?

These are removed. Instead we are now relying on the the common exchange
instructions (see instruction-scheduler.h)

https://codereview.chromium.org/2623633003/diff/40001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/40001/src/x64/assembler-x64.c...
src/x64/assembler-x64.cc:1992: if (is_signed) {
On 2017/01/17 21:45:50, binji wrote:
> This is the wrong place to put this code; the assembler is meant to map
> more-or-less directly to instructions.

Removed. Moved the move part upstream in code-generator-x64 instead.

[aseemgarg, 2017-02-21 13:57:27]

This can go in after https://codereview.chromium.org/2711473002/

[aseemgarg, 2017-02-21 22:42:33]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 22:42:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 22:44:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 22:44:12]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35282)

[aseemgarg, 2017-02-21 23:19:41]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 23:19:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 23:23:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:23:01]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[aseemgarg, 2017-02-21 23:47:47]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 23:47:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 23:52:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:52:42]

Dry run: Try jobs failed on following builders:
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[binji, 2017-02-22 02:08:38]

looks pretty good.

As we discussed offline, see if you can continue to use the CPP builtin for
mips/mips64.

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:535: __ Dmb(InnerShareable,
BarrierAll);                                \
I don't think the barrier is required

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:541: __ Cmp(i.TempRegister32(),
Operand(0));                            \
Could use Cbnz?

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/ia32/code...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/ia32/code...
src/compiler/ia32/code-generator-ia32.cc:1940: Operand operand =
i.MemoryOperand(&index);
looks like if index is not used afterward it is simpler to just use
i.InputRegister(0) and i.MemoryOperand(1), then you can ditch the macro (as it
doesn't seem to be a common pattern in this file)

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:2217: Register r =
i.InputRegister(index++); \
same comment here as for ia32

[aseemgarg, 2017-02-22 03:29:02]

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:535: __ Dmb(InnerShareable,
BarrierAll);                                \
On 2017/02/22 02:08:38, binji wrote:
> I don't think the barrier is required

Done.

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:541: __ Cmp(i.TempRegister32(),
Operand(0));                            \
On 2017/02/22 02:08:38, binji wrote:
> Could use Cbnz?

Done.

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/ia32/code...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/ia32/code...
src/compiler/ia32/code-generator-ia32.cc:1940: Operand operand =
i.MemoryOperand(&index);
On 2017/02/22 02:08:38, binji wrote:
> looks like if index is not used afterward it is simpler to just use
> i.InputRegister(0) and i.MemoryOperand(1), then you can ditch the macro (as it
> doesn't seem to be a common pattern in this file)

Done.

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2623633003/diff/260001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:2217: Register r =
i.InputRegister(index++); \
On 2017/02/22 02:08:38, binji wrote:
> same comment here as for ia32

Done.

[aseemgarg, 2017-02-22 03:29:45]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 03:29:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 04:23:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 04:23:21]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-02-24 13:42:06]

The CQ bit was checked by aseemgarg@chromium.org

[commit-bot: I haz the power, 2017-02-24 13:42:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 13:42:12]

This CL has an open dependency (Issue 2711473002 Patch 60001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[aseemgarg, 2017-02-24 13:42:37]

The CQ bit was unchecked by aseemgarg@chromium.org

[aseemgarg, 2017-02-24 13:42:43]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 13:42:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 14:13:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 14:13:34]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-02-27 07:06:46]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 07:06:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 07:35:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 07:35:23]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-03-01 03:03:11]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 03:03:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aseemgarg, 2017-03-01 03:11:49]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 03:11:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aseemgarg, 2017-03-01 03:25:18]

Description was changed from

==========
[Atomics] Make Atomics.exchange a builtin using TF

BUG=v8:4614
R=binji@chromium.org
==========

to

==========
[Atomics] Make Atomics.exchange a builtin using TF

BUG=v8:4614
R=binji@chromium.org
==========

[aseemgarg, 2017-03-01 03:25:18]

aseemgarg@chromium.org changed reviewers:
+ jarin@chromium.org

[binji, 2017-03-01 03:30:16]

lgtm

jarin: we don't have MIPS/MIPS64 implementation in TFJ currently (missing
simulator, assembler ops, etc.). What do you think about keeping that
implementation in C++ for now?

[commit-bot: I haz the power, 2017-03-01 03:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 03:50:24]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-03-01 07:45:37]

https://codereview.chromium.org/2623633003/diff/400001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2623633003/diff/400001/src/builtins/builtins....
src/builtins/builtins.h:54: CPP(AtomicsExchange)
Rather than special casing here, could you keep the runtime function and let the
mips stub tailcall into the runtime function?

Even better, could you ping v8-mips-ports to help you with the mips part?

https://codereview.chromium.org/2623633003/diff/400001/src/compiler/instructi...
File src/compiler/instruction-scheduler.cc (right):

https://codereview.chromium.org/2623633003/diff/400001/src/compiler/instructi...
src/compiler/instruction-scheduler.cc:334: return kIsLoadOperation |
kHasSideEffect;
kIsLoadOperation is redundant here, kHasSideEffect already means that we can
reorder it with any load or side-effect.

[aseemgarg, 2017-03-02 03:26:56]

https://codereview.chromium.org/2623633003/diff/400001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2623633003/diff/400001/src/builtins/builtins....
src/builtins/builtins.h:54: CPP(AtomicsExchange)
On 2017/03/01 07:45:37, Jarin wrote:
> Rather than special casing here, could you keep the runtime function and let
the
> mips stub tailcall into the runtime function?
> 
> Even better, could you ping v8-mips-ports to help you with the mips part?

Put in call to runtime function. Will ping v8-mips-ports once this patch is
submitted.

https://codereview.chromium.org/2623633003/diff/400001/src/compiler/instructi...
File src/compiler/instruction-scheduler.cc (right):

https://codereview.chromium.org/2623633003/diff/400001/src/compiler/instructi...
src/compiler/instruction-scheduler.cc:334: return kIsLoadOperation |
kHasSideEffect;
On 2017/03/01 07:45:37, Jarin wrote:
> kIsLoadOperation is redundant here, kHasSideEffect already means that we can
> reorder it with any load or side-effect.

Done.

[aseemgarg, 2017-03-02 03:27:23]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 03:27:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 04:09:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 04:09:51]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-03-03 01:20:42]

The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 01:20:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 01:49:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 01:49:10]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-03-06 10:59:05]

lgtm

https://codereview.chromium.org/2623633003/diff/500001/src/builtins/builtins-...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2623633003/diff/500001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:297: index_integer, value_integer));
Can't you tail-call to the runtime?

I was hoping you could say here "a.TailCallRuntime(..." instead of
"a.Return(a.CallRuntime".

[aseemgarg, 2017-03-06 23:20:07]

https://codereview.chromium.org/2623633003/diff/500001/src/builtins/builtins-...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2623633003/diff/500001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:297: index_integer, value_integer));
On 2017/03/06 10:59:05, Jarin wrote:
> Can't you tail-call to the runtime?
> 
> I was hoping you could say here "a.TailCallRuntime(..." instead of
> "a.Return(a.CallRuntime".

I tried doing a.TailCallRuntime instead of a.Return(a.CallRuntime(.. However, I
keep getting an error "abort: The stack pointer is not the expected value" I
can't figure out why that's happening, so I figured would go with this since it
is supposed to be removed eventually anyways.

[aseemgarg, 2017-03-06 23:29:42]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-03-06 23:29:43]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org
Link to the patchset: https://codereview.chromium.org/2623633003/#ps500001
(title: "remove 0 extend for arm")

[commit-bot: I haz the power, 2017-03-06 23:29:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 23:33:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 23:33:31]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36062)

[aseemgarg, 2017-03-06 23:44:49]

aseemgarg@chromium.org changed reviewers:
+ adamk@chromium.org

[aseemgarg, 2017-03-06 23:47:20]

+adamk for src/js/harmony-atomics.js

[adamk, 2017-03-06 23:58:10]

lgtm

[aseemgarg, 2017-03-07 00:01:12]

The CQ bit was checked by aseemgarg@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:01:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 00:07:46]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1488844872496590,
"parent_rev": "6e520c62932640cb7abf205d3b6bb9b22df6f6a4", "commit_rev":
"301c12376e5c675c5a3d5265d1bf2b3ab1e13679"}

[commit-bot: I haz the power, 2017-03-07 00:07:54]

Description was changed from

==========
[Atomics] Make Atomics.exchange a builtin using TF

BUG=v8:4614
R=binji@chromium.org
==========

to

==========
[Atomics] Make Atomics.exchange a builtin using TF

BUG=v8:4614
R=binji@chromium.org

Review-Url: https://codereview.chromium.org/2623633003
Cr-Commit-Position: refs/heads/master@{#43623}
Committed:
https://chromium.googlesource.com/v8/v8/+/301c12376e5c675c5a3d5265d1bf2b3ab1e...
==========

[commit-bot: I haz the power, 2017-03-07 00:07:56]

Committed patchset #26 (id:500001) as
https://chromium.googlesource.com/v8/v8/+/301c12376e5c675c5a3d5265d1bf2b3ab1e...

[jbramley, 2017-03-08 11:11:06]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2017-03-08 11:11:07]

Sorry, I realise that I missed the review window, but I have one comment about
the correct use of atomics on AArch64.

https://codereview.chromium.org/2623633003/diff/500001/src/compiler/arm64/cod...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2623633003/diff/500001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:539: i.TempRegister(0));             
                   \
Acquire/release instructions shouldn't be used to control access to resources
that are also controlled via traditional barriers (like in
ASSEMBLE_ATOMIC_STORE_INTEGER above). For a given shared resource, we need to
pick one approach and use it consistently.

Ideally, these should all use acquire/release semantics since we know that
they're always available for AArch64, and theoretically they are faster.
However, the existing ones use traditional load/store+barrier sequences and
rewriting them will be a lot of work. Perhaps for now,
ASSEMBLE_ATOMIC_EXCHANGE_INTEGER should also use a dmb-based implementation.