[Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator

[Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator

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

Review-Url: https://codereview.chromium.org/2711473002
Cr-Commit-Position: refs/heads/master@{#43461}
Committed: https://chromium.googlesource.com/v8/v8/+/a2a2c1b9eeccd86e77f2c7e6eda3e1b09bb5306c

[aseemgarg, 2017-02-21 12:35:19]

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

[commit-bot: I haz the power, 2017-02-21 12:35:27]

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 13:04:33]

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

[commit-bot: I haz the power, 2017-02-21 13:04:34]

Dry run: This issue passed the CQ dry run.

[binji, 2017-02-22 01:50:16]

binji@chromium.org changed reviewers:
+ jacob.bramley@arm.com, jarin@chromium.org

[binji, 2017-02-22 01:50:17]

+jarin
+jbramley

https://codereview.chromium.org/2711473002/diff/20001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.h (right):

https://codereview.chromium.org/2711473002/diff/20001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:868: // Syncronization primitives. See ARM DDI
0487A.a, B2.10. Pair types not
sp: synchronization

[aseemgarg, 2017-02-22 03:08:25]

https://codereview.chromium.org/2711473002/diff/20001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.h (right):

https://codereview.chromium.org/2711473002/diff/20001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:868: // Syncronization primitives. See ARM DDI
0487A.a, B2.10. Pair types not
On 2017/02/22 01:50:17, binji wrote:
> sp: synchronization

Done.

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

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:33]

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:23]

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

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

Dry run: This issue passed the CQ dry run.

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

The CQ bit was checked by aseemgarg@chromium.org

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

The CQ bit was unchecked by aseemgarg@chromium.org

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

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:32]

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:09:54]

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

[commit-bot: I haz the power, 2017-02-24 14:09:55]

Dry run: This issue passed the CQ dry run.

[jbramley, 2017-02-24 15:25:41]

It's reassuringly similar to the ARM implementation, and nicely integrated.

My comments are only about style and the like. LGTM otherwise.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.cc:1961: bool is_exclusive =
!instr->LoadStoreXNotExclusive();
These helpers return integers. Usually we would test (for example)
"instr->LoadStoreXAcquireRelease() != 0" rather than rely on the implicit
conversion.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.cc:1966: DCHECK(!is_pair);      // pair unimplemented
Capital letters, full stops.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.cc:1971: base::LockGuard<base::Mutex>
lock_guard(&global_monitor_.Pointer()->mutex);
You lock the mutex in both paths, so perhaps factor it out.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.h (right):

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:869: // implemented
Missing full stop (/period).

[aseemgarg, 2017-02-24 22:38:49]

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.cc:1961: bool is_exclusive =
!instr->LoadStoreXNotExclusive();
On 2017/02/24 15:25:41, jbramley wrote:
> These helpers return integers. Usually we would test (for example)
> "instr->LoadStoreXAcquireRelease() != 0" rather than rely on the implicit
> conversion.

Done.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.cc:1966: DCHECK(!is_pair);      // pair unimplemented
On 2017/02/24 15:25:41, jbramley wrote:
> Capital letters, full stops.

Done.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.cc:1971: base::LockGuard<base::Mutex>
lock_guard(&global_monitor_.Pointer()->mutex);
On 2017/02/24 15:25:41, jbramley wrote:
> You lock the mutex in both paths, so perhaps factor it out.

Done.

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.h (right):

https://codereview.chromium.org/2711473002/diff/60001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:869: // implemented
On 2017/02/24 15:25:41, jbramley wrote:
> Missing full stop (/period).

Done.

[aseemgarg, 2017-02-24 22:40:06]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-02-24 22:40:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
jacob.bramley@arm.com
Link to the patchset: https://codereview.chromium.org/2711473002/#ps80001
(title: "comments")

[commit-bot: I haz the power, 2017-02-24 22:40:13]

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 22:43:22]

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

[commit-bot: I haz the power, 2017-02-24 22:43:23]

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/35486)

[aseemgarg, 2017-02-25 00:59:30]

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

[commit-bot: I haz the power, 2017-02-25 00:59:36]

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-25 01:28:34]

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

[commit-bot: I haz the power, 2017-02-25 01:28:35]

Dry run: This issue passed the CQ dry run.

[binji, 2017-02-27 20:02:33]

lgtm

https://codereview.chromium.org/2711473002/diff/120001/src/arm64/simulator-ar...
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/2711473002/diff/120001/src/arm64/simulator-ar...
src/arm64/simulator-arm64.cc:1961: int32_t is_exclusive =
!instr->LoadStoreXNotExclusive();
nit: == 0 instead of !

[Jarin, 2017-02-27 21:16:36]

lgtm

[aseemgarg, 2017-02-27 21:43:04]

https://codereview.chromium.org/2711473002/diff/120001/src/arm64/simulator-ar...
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/2711473002/diff/120001/src/arm64/simulator-ar...
src/arm64/simulator-arm64.cc:1961: int32_t is_exclusive =
!instr->LoadStoreXNotExclusive();
On 2017/02/27 20:02:33, binji wrote:
> nit: == 0 instead of !

Done.

[aseemgarg, 2017-02-27 21:43:11]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-02-27 21:43:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jacob.bramley@arm.com, jarin@chromium.org, binji@chromium.org
Link to the patchset: https://codereview.chromium.org/2711473002/#ps140001
(title: "[Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator")

[commit-bot: I haz the power, 2017-02-27 21:43:20]

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 22:12:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488231791901360,
"parent_rev": "f348e573c0b05e89a9581c136eba783ad11e7119", "commit_rev":
"a2a2c1b9eeccd86e77f2c7e6eda3e1b09bb5306c"}

[commit-bot: I haz the power, 2017-02-27 22:12:24]

Description was changed from

==========
[Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator

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

to

==========
[Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator

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

Review-Url: https://codereview.chromium.org/2711473002
Cr-Commit-Position: refs/heads/master@{#43461}
Committed:
https://chromium.googlesource.com/v8/v8/+/a2a2c1b9eeccd86e77f2c7e6eda3e1b09bb...
==========

[commit-bot: I haz the power, 2017-02-27 22:12:25]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/a2a2c1b9eeccd86e77f2c7e6eda3e1b09bb...

[Dan Ehrenberg, 2017-02-27 22:47:00]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2720133003/ by littledan@chromium.org.

The reason for reverting is: The tree is closed due to an msan violation (use of
uninitialized value) in the arm64 simulator soon after this patch landed; this
seems related

https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20....