Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1889693003:
PPC: Fix atomic load sequence. (Closed)
|
Created:
4 years, 8 months ago by MTBrandyberry Modified:
4 years, 8 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionPPC: Fix atomic load sequence.
R=binji@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com, bjaideep@ca.ibm.com
BUG=
Committed: https://crrev.com/d99baa26885677afa25ce5b2638e61b0582a132d
Cr-Commit-Position: refs/heads/master@{#35503}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Defeat branch+4 optimization. #Messages
Total messages: 18 (6 generated)
PTAL
The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889693003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889693003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1889693003/diff/1/src/ppc/code-stubs-ppc.cc File src/ppc/code-stubs-ppc.cc (right): https://codereview.chromium.org/1889693003/diff/1/src/ppc/code-stubs-ppc.cc#n... src/ppc/code-stubs-ppc.cc:5917: __ bne(&dummy); \ I'm not very familiar with sync on PPC. What's the purpose of this bne? Isn't the target and fallthrough the same (i.e. dummy)? How does one get out of this loop?
https://codereview.chromium.org/1889693003/diff/1/src/ppc/code-stubs-ppc.cc File src/ppc/code-stubs-ppc.cc (right): https://codereview.chromium.org/1889693003/diff/1/src/ppc/code-stubs-ppc.cc#n... src/ppc/code-stubs-ppc.cc:5917: __ bne(&dummy); \ On 2016/04/14 17:32:09, JoranSiu wrote: > I'm not very familiar with sync on PPC. What's the purpose of this bne? > Isn't the target and fallthrough the same (i.e. dummy)? How does one get out of > this loop? Ignore the loop part of my question.. :) I'm still curious about why we need to have the cmp/bne/label.
On 2016/04/14 17:39:16, JoranSiu wrote: > https://codereview.chromium.org/1889693003/diff/1/src/ppc/code-stubs-ppc.cc > File src/ppc/code-stubs-ppc.cc (right): > > https://codereview.chromium.org/1889693003/diff/1/src/ppc/code-stubs-ppc.cc#n... > src/ppc/code-stubs-ppc.cc:5917: __ bne(&dummy); > \ > On 2016/04/14 17:32:09, JoranSiu wrote: > > I'm not very familiar with sync on PPC. What's the purpose of this bne? > > Isn't the target and fallthrough the same (i.e. dummy)? How does one get out > of > > this loop? > > Ignore the loop part of my question.. :) I'm still curious about why we need > to have the cmp/bne/label. see https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html -- load seq cst sequence also http://www.ibm.com/developerworks/systems/articles/powerpc.html I'm going to amend the commit to make it clear that that branch is not taken
The CQ bit was checked by mbrandy@us.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889693003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889693003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I suspect there's probably a more optimal sequence, but we can revisit this in a future CL. The change lgtm otherwise.
The CQ bit was checked by mbrandy@us.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889693003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889693003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== PPC: Fix atomic load sequence. R=binji@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com, bjaideep@ca.ibm.com BUG= ========== to ========== PPC: Fix atomic load sequence. R=binji@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com, bjaideep@ca.ibm.com BUG= Committed: https://crrev.com/d99baa26885677afa25ce5b2638e61b0582a132d Cr-Commit-Position: refs/heads/master@{#35503} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d99baa26885677afa25ce5b2638e61b0582a132d Cr-Commit-Position: refs/heads/master@{#35503} |
