[Atomics] Make Atomics.compareExchange a builtin using TF

[Atomics] Make Atomics.compareExchange a builtin using TF

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

Review-Url: https://codereview.chromium.org/2649703002
Cr-Commit-Position: refs/heads/master@{#43878}
Committed: https://chromium.googlesource.com/v8/v8/+/82b5c8c9440781976ce23cf8fcabd250ef2bee02

[aseemgarg, 2017-01-21 13:27:20]

There is a bug somewhere that is causing failures for Int16 and Int8 types.

[aseemgarg, 2017-02-24 13:39:54]

The CQ bit was checked by aseemgarg@chromium.org

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

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

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

This CL has an open dependency (Issue 2623633003 Patch 320001). 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:40:10]

The CQ bit was unchecked by aseemgarg@chromium.org

[aseemgarg, 2017-02-24 13:40:24]

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:40:31]

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

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

[commit-bot: I haz the power, 2017-02-24 15:23:30]

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

[aseemgarg, 2017-02-26 08:48:55]

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

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

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-26 08:50:33]

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

[commit-bot: I haz the power, 2017-02-26 08:50:34]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21625)
  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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18093)

[aseemgarg, 2017-02-26 09:41:12]

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

[commit-bot: I haz the power, 2017-02-26 09:41:17]

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-26 11:25:34]

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

[commit-bot: I haz the power, 2017-02-26 11:25:35]

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

[aseemgarg, 2017-02-27 06:57:37]

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

[commit-bot: I haz the power, 2017-02-27 06:57:46]

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

[binji, 2017-02-28 23:28:53]

https://codereview.chromium.org/2649703002/diff/100001/src/compiler/arm/code-...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2649703002/diff/100001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:441: __ dmb(ISH);                        
                                     \
The optimized code is a bit hard to follow, but the idea is that you can do an
initial zero-extend on the new_value, then ldrexb (which will also zero-extend).
Then you can just do a normal cmp/bne without any of the masking.


  uxtb    r6, r2
  ldrexb  r0, [r1]
  cmp     r0, r6
  bne.n   fail
  dmb     ish
loop:
  strexb  r5, r3, [r1]
  cmp     r5, #0
  beq.w   ok
  ldrexb  r0, [r1]
  cmp     r0, r6
  beq.n   loop
ok:
  clrex
  movs    r1, #0
  dmb     ish
  b.n     done
  ...
fail:
  dmb ish
  movs    r1, #1
done:
  cmp     r1, #0
  it      ne
  movne   r0, r2
  sxtb    r0, r0

It's also worth noting that they only loop while the comparison is still valid.
This is a nice optimization, but I think we can do something simpler and just do
one iteration, rather than loop at all. Basically, something like this:

old_extend = uxt(old);
read = ldrex(addr)
if (read == old_extend) {
  strex(addr, new);
} else {
  clrex;
}
return sign_or_zero_extend(read);

[aseemgarg, 2017-03-03 01:34:13]

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:34:18]

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-03 01:38:32]

Description was changed from

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

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

to

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

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

[aseemgarg, 2017-03-03 01:38:32]

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

[commit-bot: I haz the power, 2017-03-03 02:00:48]

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

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

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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[aseemgarg, 2017-03-07 01:21:17]

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

[aseemgarg, 2017-03-07 01:21:27]

The CQ bit was checked by aseemgarg@chromium.org

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

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 01:21:35]

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

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

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[aseemgarg, 2017-03-07 01:22:02]

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

[commit-bot: I haz the power, 2017-03-07 01:22:12]

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-07 01:58:22]

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

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

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

[aseemgarg, 2017-03-08 22:31:43]

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

[commit-bot: I haz the power, 2017-03-08 22:31:45]

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-08 22:58:31]

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

[commit-bot: I haz the power, 2017-03-08 22:58:32]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-03-08 23:01:30]

Please review.

[Jakob Kummerow, 2017-03-09 13:43:42]

DBC.

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

https://codereview.chromium.org/2649703002/diff/200001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:350: void
Builtins::Generate_AtomicsCompareExchange(
please use the TF_BUILTIN macro to drop a bunch of visual clutter, see
https://codereview.chromium.org/2734323004

https://codereview.chromium.org/2649703002/diff/200001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:430: a.Return(a.SmiConstant(0));
how about "Unreachable();" instead?

[aseemgarg, 2017-03-10 19:55:11]

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

[aseemgarg, 2017-03-10 19:55:22]

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

https://codereview.chromium.org/2649703002/diff/200001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:350: void
Builtins::Generate_AtomicsCompareExchange(
On 2017/03/09 13:43:42, Jakob Kummerow wrote:
> please use the TF_BUILTIN macro to drop a bunch of visual clutter, see
> https://codereview.chromium.org/2734323004

Done.

https://codereview.chromium.org/2649703002/diff/200001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:430: a.Return(a.SmiConstant(0));
On 2017/03/09 13:43:42, Jakob Kummerow wrote:
> how about "Unreachable();" instead?

Done.

[aseemgarg, 2017-03-10 19:55:36]

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

[commit-bot: I haz the power, 2017-03-10 19:55:38]

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-10 20:00:27]

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

[commit-bot: I haz the power, 2017-03-10 20:00:28]

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

[Jakob Kummerow, 2017-03-10 21:05:39]

Thanks, src/builtins/* LGTM with nits.

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

https://codereview.chromium.org/2649703002/diff/220001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:364:
a.Return(a.CallRuntime(Runtime::kAtomicsCompareExchange, context, array,
s/a.//g

https://codereview.chromium.org/2649703002/diff/220001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:373: CodeStubAssembler::Label
i8(this), u8(this), i16(this), u16(this), i32(this),
s/CodeStubAssembler:://

https://codereview.chromium.org/2649703002/diff/220001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:379: CodeStubAssembler::Label*
case_labels[] = {
s/CodeStubAssembler:://

[aseemgarg, 2017-03-10 21:09:54]

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

https://codereview.chromium.org/2649703002/diff/220001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:364:
a.Return(a.CallRuntime(Runtime::kAtomicsCompareExchange, context, array,
On 2017/03/10 21:05:39, Jakob Kummerow wrote:
> s/a.//g

Done.

https://codereview.chromium.org/2649703002/diff/220001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:373: CodeStubAssembler::Label
i8(this), u8(this), i16(this), u16(this), i32(this),
On 2017/03/10 21:05:39, Jakob Kummerow wrote:
> s/CodeStubAssembler:://

Done.

https://codereview.chromium.org/2649703002/diff/220001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:379: CodeStubAssembler::Label*
case_labels[] = {
On 2017/03/10 21:05:39, Jakob Kummerow wrote:
> s/CodeStubAssembler:://

Done.

[aseemgarg, 2017-03-10 21:10:08]

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

[commit-bot: I haz the power, 2017-03-10 21:10:20]

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-10 21:31:11]

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

[commit-bot: I haz the power, 2017-03-10 21:31:12]

Dry run: This issue passed the CQ dry run.

[binji, 2017-03-10 23:08:03]

One Q about ARM64, otherwise lgtm

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

https://codereview.chromium.org/2649703002/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1688:
ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldaxrb, stlxrb);
Is it necessary to zero-extend the output register here, or does the ldaxrb do
that already?

[aseemgarg, 2017-03-10 23:14:32]

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

https://codereview.chromium.org/2649703002/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1688:
ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldaxrb, stlxrb);
On 2017/03/10 23:08:02, binji wrote:
> Is it necessary to zero-extend the output register here, or does the ldaxrb do
> that already?

ldaxrb/ldaxrh zero extends always. So, we need to take care for signed cases but
not unsigned cases.

[aseemgarg, 2017-03-11 00:45:43]

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

https://codereview.chromium.org/2649703002/diff/260001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:1688:
ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldaxrb, stlxrb);
On 2017/03/10 23:14:32, aseemgarg wrote:
> On 2017/03/10 23:08:02, binji wrote:
> > Is it necessary to zero-extend the output register here, or does the ldaxrb
do
> > that already?
> 
> ldaxrb/ldaxrh zero extends always. So, we need to take care for signed cases
but
> not unsigned cases.

It is verified that nodes that require 64bit input do not directly use 32bit
output from another node. So, ldaxrb will correctly zero extend to 32 bit and if
that value is needed by a 64 bit operator, it will have to go through a casting
operation.

[aseemgarg, 2017-03-13 23:55:28]

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

[commit-bot: I haz the power, 2017-03-13 23:55:40]

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-13 23:58:14]

Please review. Also, we have runtime function for compareExchange (and exchange)
for mips, ppc and S390. Does this seem fine in short term (before those archs
are fixed) or should we do something different?

[commit-bot: I haz the power, 2017-03-14 00:18:11]

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

[commit-bot: I haz the power, 2017-03-14 00:18: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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Jarin, 2017-03-14 09:37:27]

lgtm

[aseemgarg, 2017-03-15 23:25:38]

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

[commit-bot: I haz the power, 2017-03-15 23:25:42]

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-16 00:43:01]

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

[commit-bot: I haz the power, 2017-03-16 00:43:02]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-03-16 22:00:26]

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

[commit-bot: I haz the power, 2017-03-16 22:00:37]

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-16 22:26:25]

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

[commit-bot: I haz the power, 2017-03-16 22:26:29]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-03-16 22:26:57]

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

https://codereview.chromium.org/2649703002/diff/320001/src/compiler/arm64/cod...
src/compiler/arm64/code-generator-arm64.cc:547: #define
ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(load_instr, store_instr) \
Landing this for now with ldaxr and stlxr use. Will make a change to dmb based
impl. if needed once https://codereview.chromium.org/2754543006/ is resolved and
landed.

[aseemgarg, 2017-03-16 22:27:06]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-03-16 22:27:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org, binji@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2649703002/#ps320001
(title: "rebase and move cmpxchg to builtins-sharedarraybuffer-gen.cc")

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

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-16 22:29:04]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1489703226754020,
"parent_rev": "469b632e6599d518f83c30bf9f0d5f2f53c4ae15", "commit_rev":
"82b5c8c9440781976ce23cf8fcabd250ef2bee02"}

[commit-bot: I haz the power, 2017-03-16 22:29:11]

Description was changed from

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

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

to

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

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

Review-Url: https://codereview.chromium.org/2649703002
Cr-Commit-Position: refs/heads/master@{#43878}
Committed:
https://chromium.googlesource.com/v8/v8/+/82b5c8c9440781976ce23cf8fcabd250ef2...
==========

[commit-bot: I haz the power, 2017-03-16 22:29:12]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/v8/v8/+/82b5c8c9440781976ce23cf8fcabd250ef2...