Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by aseemgarg Modified:
3 years, 9 months ago Reviewers:
Jarin, jbramley, binji CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[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
Patch Set 1 #Patch Set 2 : [Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator #
Total comments: 2
Patch Set 3 : [Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator #Patch Set 4 : rebase #
Total comments: 8
Patch Set 5 : comments #Patch Set 6 : rebase #Patch Set 7 : [Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator #
Total comments: 2
Patch Set 8 : [Atomics] Implement ldaxr/stlxr instructions in ARM64 simulator #
Created: 3 years, 9 months ago
Depends on Patchset: Messages
Total messages: 39 (26 generated)
The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
binji@chromium.org changed reviewers: + jacob.bramley@arm.com, jarin@chromium.org
+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
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.
The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aseemgarg@chromium.org
The CQ bit was unchecked by aseemgarg@chromium.org
The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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).
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.
The CQ bit was checked by aseemgarg@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 !
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(); On 2017/02/27 20:02:33, binji wrote: > nit: == 0 instead of ! Done.
The CQ bit was checked by aseemgarg@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488231791901360,
"parent_rev": "f348e573c0b05e89a9581c136eba783ad11e7119", "commit_rev":
"a2a2c1b9eeccd86e77f2c7e6eda3e1b09bb5306c"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/a2a2c1b9eeccd86e77f2c7e6eda3e1b09bb...
Message was sent while issue was closed.
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.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
