[Atomics] code stubs for atomic operations

[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
LOG=y

Committed: https://crrev.com/10b5febe11b318cfef130abae343183ac862e60d
Cr-Commit-Position: refs/heads/master@{#35427}

[binji, 2016-01-27 20:29:27]

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

[binji, 2016-01-27 20:29:27]

+jarin: review
+jfb, bradnelson: FYI

[Jarin, 2016-01-28 08:11:12]

The stubs look great (modulo minor nits), but we need to think harder
about wiring this up. The current approach with intrinsics has two
problems:

- Normally, intrinsics should have a fallback in C++, which is not the
  case here. This is not a show-stopper, but it does mean that you
  would have to provide a separate implementation for the interpreter.

- The bigger problem is that we won't really be able to optimize this
  properly in its current form because AtomicsLoadJS calls
  %IsSharedIntegerTypedArray and this still goes to C++. Even if it
  did not go there, AtomicsLoadJS is really hard for an optimizing
  compiler.

I talked to Benedikt about this and he would like to see different
wiring that fixes both the problems: instead of the JavaScript
implementation (AtomicLoadJS), we could introduce a builtin for the
load that would implement AtomicLoadJS in native code. There are two
ways to do this:

- Write the code by hand for each platform (e.g., see
  src/x64/builtins-x64.(h|cc)) and register it during bootstrap (see
  bootstrapper.cc, search for SimpleInstallFunction).

- If you are looking for adventure (or strongly dislike writing
  platform-specific code), you could have a go at writing the builtins
  in Turbofan, using code-stub-assembler.h. The infrastructure is
  very much work in progress, but it would be awesome if we had
  someone who would try to use that.

In both cases, you could still use the stub; you would 'only' write
new code for all the checks and conversions that surround the actual
atomic access.

That said, I think you could also tweak your current intrinsic
plumbing, but you would have to find a way to make it work with the
interpreter.  AFAIK, interpreter does not easily support calls to
arbitrary stubs.  The call itself is not a problem, but there is
limited number of opcodes and the encoding for intrinsics has not been
designed yet.

In any case, you will want to be more clever in Turbofan. If you do
the builtin approach, you would not want to pay the overhead of
checks/conversions if you already know the types statically. With the
current approach based on intrinsics you would definitely want to
avoid the overhead of runtime call to  %IsSharedIntegerTypedArray, and
you would probably have to do something about the conversions - I do
not think Turbofan would be quite able to elide all of them even if
the types are statically known.

CC'ing @bmeurer to check that I am not talking nonsense, and @mcilroy
for the interpreter bit.

https://codereview.chromium.org/1617503003/diff/1/src/full-codegen/x64/full-c...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1617503003/diff/1/src/full-codegen/x64/full-c...
src/full-codegen/x64/full-codegen-x64.cc:4672: void
FullCodeGenerator::EmitAtomicsLoad(CallRuntime* expr) {
You can use FullCodeGenerator::EmitIntrinsicAsStubCall (in full-codegen.cc) to
avoid the boilerplate code.

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5447: static void
GetTypedArrayBackingStore(MacroAssembler* masm,
Nit: Anonymous namespace for all the static methods, please.

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5459: // offset is smi
Nit: If a comment is a sentence, it should start with a capital letter and end
with a dot.

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5505: STATIC_ASSERT(FIXED_UINT8_CLAMPED_ARRAY_TYPE ==
FIXED_INT8_ARRAY_TYPE + 8);
I like this. Thanks!

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5528: __ b(u8);      // Uint8ClampedArray
This should be u8c, no? (Also on all other platforms.)

https://codereview.chromium.org/1617503003/diff/60002/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:3908: // TODO(binji): implement
How about having a global variable for the fences? Then we would atomically
increment it by 0 here. This should give us strong-enough guarantees for the
barriers, I think.

https://codereview.chromium.org/1617503003/diff/60002/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:25: static CallDescriptor::Flags
AdjustFrameStatesForCall(Node* node) {
I believe here you only need a simpler version because the frame state count
should be always > 0. 

Do you ever see count == 0 here? 

If no, then maybe DCHECK(count > 0) and you do not have to pass the flags in the
methods below.

Actually, thinking about it more, your stubs should never cause lazy deopt, so
maybe we could try to wire this up without frame states. In any case, you should
know statically whether you need a frame state or not.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/arm/lith...
File src/crankshaft/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/arm/lith...
src/crankshaft/arm/lithium-codegen-arm.cc:996: case CodeStub::AtomicsLoad: {
I think you should use the HCallWithDescriptor hydrogen instruction, then you
would not have to do this.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:13006: HCallStub* result =
New<HCallStub>(CodeStub::AtomicsLoad, 2);
I would use HCallWithDescriptor. For example, see
HOptimizedGraphBuilder::GenerateToLength.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/ia32/lit...
File src/crankshaft/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/ia32/lit...
src/crankshaft/ia32/lithium-codegen-ia32.cc:1000: case CodeStub::AtomicsLoad: {
You should not need this if you used HCallWithDesriptor.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/x64/lith...
File src/crankshaft/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/x64/lith...
src/crankshaft/x64/lithium-codegen-x64.cc:930: case CodeStub::AtomicsLoad: {
You should not need this if you used HCallWithDesriptor.

https://codereview.chromium.org/1617503003/diff/60002/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5821: __ dd(u8);      // Uint8ClampedArray
u8c?

[Jarin, 2016-01-28 08:11:36]

The stubs look great (modulo minor nits), but we need to think harder
about wiring this up. The current approach with intrinsics has two
problems:

- Normally, intrinsics should have a fallback in C++, which is not the
  case here. This is not a show-stopper, but it does mean that you
  would have to provide a separate implementation for the interpreter.

- The bigger problem is that we won't really be able to optimize this
  properly in its current form because AtomicsLoadJS calls
  %IsSharedIntegerTypedArray and this still goes to C++. Even if it
  did not go there, AtomicsLoadJS is really hard for an optimizing
  compiler.

I talked to Benedikt about this and he would like to see different
wiring that fixes both the problems: instead of the JavaScript
implementation (AtomicLoadJS), we could introduce a builtin for the
load that would implement AtomicLoadJS in native code. There are two
ways to do this:

- Write the code by hand for each platform (e.g., see
  src/x64/builtins-x64.(h|cc)) and register it during bootstrap (see
  bootstrapper.cc, search for SimpleInstallFunction).

- If you are looking for adventure (or strongly dislike writing
  platform-specific code), you could have a go at writing the builtins
  in Turbofan, using code-stub-assembler.h. The infrastructure is
  very much work in progress, but it would be awesome if we had
  someone who would try to use that.

In both cases, you could still use the stub; you would 'only' write
new code for all the checks and conversions that surround the actual
atomic access.

That said, I think you could also tweak your current intrinsic
plumbing, but you would have to find a way to make it work with the
interpreter.  AFAIK, interpreter does not easily support calls to
arbitrary stubs.  The call itself is not a problem, but there is
limited number of opcodes and the encoding for intrinsics has not been
designed yet.

In any case, you will want to be more clever in Turbofan. If you do
the builtin approach, you would not want to pay the overhead of
checks/conversions if you already know the types statically. With the
current approach based on intrinsics you would definitely want to
avoid the overhead of runtime call to  %IsSharedIntegerTypedArray, and
you would probably have to do something about the conversions - I do
not think Turbofan would be quite able to elide all of them even if
the types are statically known.

CC'ing @bmeurer to check that I am not talking nonsense, and @mcilroy
for the interpreter bit.

[Jarin, 2016-01-28 08:14:07]

One more thing, please provide a description for this CL - the CL does a lot of
things, it would be nice to have a summary of all the stuff. 

(And I am sorry for the double-send.)

[Jarin, 2016-01-28 08:17:08]

Description was changed from

==========
[Atomics] code stubs for atomic operations

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

BUG=v8:4614
==========

[binji, 2016-01-28 16:20:01]

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5447: static void
GetTypedArrayBackingStore(MacroAssembler* masm,
On 2016/01/28 08:11:12, Jarin wrote:
> Nit: Anonymous namespace for all the static methods, please.

Happy to do it, but it doesn't seem to be consistent with the rest of the file.
Should I change the others too?

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5459: // offset is smi
On 2016/01/28 08:11:12, Jarin wrote:
> Nit: If a comment is a sentence, it should start with a capital letter and end
> with a dot.

Done.

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5505: STATIC_ASSERT(FIXED_UINT8_CLAMPED_ARRAY_TYPE ==
FIXED_INT8_ARRAY_TYPE + 8);
On 2016/01/28 08:11:12, Jarin wrote:
> I like this. Thanks!

Yeah, it's a bit paranoid, but doesn't really hurt. :)

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5528: __ b(u8);      // Uint8ClampedArray
On 2016/01/28 08:11:12, Jarin wrote:
> This should be u8c, no? (Also on all other platforms.)

oops. In this case it is fine (because AtomicLoad is the same for u8 and u8c)
but it will break for all the others.

https://codereview.chromium.org/1617503003/diff/60002/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:3908: // TODO(binji): implement
On 2016/01/28 08:11:12, Jarin wrote:
> How about having a global variable for the fences? Then we would atomically
> increment it by 0 here. This should give us strong-enough guarantees for the
> barriers, I think.

Maybe... looking at http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html, the
sequence for ARM we'll generate is:

  LoadSeqCst: ldr; dmb
  StoreSeqCst: dmb; str; dmb

it looks like we could translate dmb -> mfence:

  LoadSeqCst: mov; mfence
  StoreSeqCst: mfence; mov; mfence

This would then match the x86 SeqCst alternate implementation in that doc (with
extra fences):

  LoadSeqCst: mov
  StoreSeqCst: mov; mfence

But I'm not sure the atomic increment (probably xadd 0...?) would have the same
guarantees. What do you think?

https://codereview.chromium.org/1617503003/diff/60002/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:25: static CallDescriptor::Flags
AdjustFrameStatesForCall(Node* node) {
On 2016/01/28 08:11:12, Jarin wrote:
> I believe here you only need a simpler version because the frame state count
> should be always > 0. 
> 
> Do you ever see count == 0 here?

No, I think I was ending up with multiple frame states.
 
> 
> If no, then maybe DCHECK(count > 0) and you do not have to pass the flags in
the
> methods below.
> 
> Actually, thinking about it more, your stubs should never cause lazy deopt, so
> maybe we could try to wire this up without frame states. In any case, you
should
> know statically whether you need a frame state or not.

Can't say I fully understand this. I just found that when I tried to call the
stub without this, I was hitting an assert due to mismatching input counts.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/arm/lith...
File src/crankshaft/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/arm/lith...
src/crankshaft/arm/lithium-codegen-arm.cc:996: case CodeStub::AtomicsLoad: {
On 2016/01/28 08:11:12, Jarin wrote:
> I think you should use the HCallWithDescriptor hydrogen instruction, then you
> would not have to do this.

Thanks!

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.cc:13006: HCallStub* result =
New<HCallStub>(CodeStub::AtomicsLoad, 2);
On 2016/01/28 08:11:12, Jarin wrote:
> I would use HCallWithDescriptor. For example, see
> HOptimizedGraphBuilder::GenerateToLength.

Done.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/ia32/lit...
File src/crankshaft/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/ia32/lit...
src/crankshaft/ia32/lithium-codegen-ia32.cc:1000: case CodeStub::AtomicsLoad: {
On 2016/01/28 08:11:12, Jarin wrote:
> You should not need this if you used HCallWithDesriptor.

Done.

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/x64/lith...
File src/crankshaft/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/crankshaft/x64/lith...
src/crankshaft/x64/lithium-codegen-x64.cc:930: case CodeStub::AtomicsLoad: {
On 2016/01/28 08:11:12, Jarin wrote:
> You should not need this if you used HCallWithDesriptor.

Done.

https://codereview.chromium.org/1617503003/diff/60002/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5821: __ dd(u8);      // Uint8ClampedArray
On 2016/01/28 08:11:12, Jarin wrote:
> u8c?

Done.

[binji, 2016-01-28 16:25:06]

Description was changed from

==========
[Atomics] code stubs for atomic operations

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

* Add dmb, dsb, isb instructions to ARM disassembler, simulator
* Add test for new barrier instructions
* New atomic code stubs for x64, ia32, ARM
* Wire up code stubs to FCG, CS and TF
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

BUG=v8:4614
==========

[binji, 2016-01-28 16:49:04]

On 2016/01/28 08:11:36, Jarin wrote:
> The stubs look great (modulo minor nits), but we need to think harder
> about wiring this up. The current approach with intrinsics has two
> problems:
> 
> - Normally, intrinsics should have a fallback in C++, which is not the
>   case here. This is not a show-stopper, but it does mean that you
>   would have to provide a separate implementation for the interpreter.
> 
> - The bigger problem is that we won't really be able to optimize this
>   properly in its current form because AtomicsLoadJS calls
>   %IsSharedIntegerTypedArray and this still goes to C++. Even if it
>   did not go there, AtomicsLoadJS is really hard for an optimizing
>   compiler.

Right, I guess I assumed there was some way to magically bypass AtomicsLoadJS
when compiling with TF.

> 
> I talked to Benedikt about this and he would like to see different
> wiring that fixes both the problems: instead of the JavaScript
> implementation (AtomicLoadJS), we could introduce a builtin for the
> load that would implement AtomicLoadJS in native code. There are two
> ways to do this:
> 
> - Write the code by hand for each platform (e.g., see
>   src/x64/builtins-x64.(h|cc)) and register it during bootstrap (see
>   bootstrapper.cc, search for SimpleInstallFunction).
> 
> - If you are looking for adventure (or strongly dislike writing
>   platform-specific code), you could have a go at writing the builtins
>   in Turbofan, using code-stub-assembler.h. The infrastructure is
>   very much work in progress, but it would be awesome if we had
>   someone who would try to use that.
> 
> In both cases, you could still use the stub; you would 'only' write
> new code for all the checks and conversions that surround the actual
> atomic access.

OK, this sounds doable. Sure, I'll take a look at being the guinea pig^W^W^W^W
generating the builtin from TF. :)

It sounds like it is worth going ahead and doing it in this CL?

> 
> That said, I think you could also tweak your current intrinsic
> plumbing, but you would have to find a way to make it work with the
> interpreter.  AFAIK, interpreter does not easily support calls to
> arbitrary stubs.  The call itself is not a problem, but there is
> limited number of opcodes and the encoding for intrinsics has not been
> designed yet.

It sounds like we'll have to come up with a better solution for the interpreter
either way? Or are you saying that the interpreter will correctly call the
builtin but not the intrinsic without allocating an opcode?

> 
> In any case, you will want to be more clever in Turbofan. If you do
> the builtin approach, you would not want to pay the overhead of
> checks/conversions if you already know the types statically. With the
> current approach based on intrinsics you would definitely want to
> avoid the overhead of runtime call to  %IsSharedIntegerTypedArray, and
> you would probably have to do something about the conversions - I do
> not think Turbofan would be quite able to elide all of them even if
> the types are statically known.

Yes, I assumed I'd have to do more work in TF. I added what's there now just so
TF worked at all. I figure I can work on the optimized version in another CL,
wdyt?

> 
> CC'ing @bmeurer to check that I am not talking nonsense, and @mcilroy
> for the interpreter bit.

[rmcilroy, 2016-01-28 18:25:29]

> It sounds like we'll have to come up with a better solution for the
interpreter
> either way? Or are you saying that the interpreter will correctly call the
> builtin but not the intrinsic without allocating an opcode?

Yes, the interpreter currently relies on the fact that every intrinsic we need
currently had a C++ runtime function implementation, and we call that directly
(with the CallRuntime bytecode). We are planning on adding a mechanism to for
intrinsics in the somewhat near future but it'll be a few weeks before we start
on it. 

If you implement the stubs in TF using the code stub assembler then we can
probably adapt it such that it can be inlined directly in the bytecode handler,
otherwise we can just have the bytecode handler call the stub directly.

[Jarin, 2016-02-01 07:35:48]

On 2016/01/28 16:49:04, binji wrote:
> On 2016/01/28 08:11:36, Jarin wrote:
> > The stubs look great (modulo minor nits), but we need to think harder
> > about wiring this up. The current approach with intrinsics has two
> > problems:
> > 
> > - Normally, intrinsics should have a fallback in C++, which is not the
> >   case here. This is not a show-stopper, but it does mean that you
> >   would have to provide a separate implementation for the interpreter.
> > 
> > - The bigger problem is that we won't really be able to optimize this
> >   properly in its current form because AtomicsLoadJS calls
> >   %IsSharedIntegerTypedArray and this still goes to C++. Even if it
> >   did not go there, AtomicsLoadJS is really hard for an optimizing
> >   compiler.
> 
> Right, I guess I assumed there was some way to magically bypass AtomicsLoadJS
> when compiling with TF.

Actually, you are right. There is js-builtin-reducer, my point was not
really valid. It would be still a bit strange to have an optimized
intrinsic, but on the same code path have a C++ call (but I do understand
that in this case we have an intrinsic just to have better control over
the generated code sequence for atomic, not for efficiency).

> > I talked to Benedikt about this and he would like to see different
> > wiring that fixes both the problems: instead of the JavaScript
> > implementation (AtomicLoadJS), we could introduce a builtin for the
> > load that would implement AtomicLoadJS in native code. There are two
> > ways to do this:
> > 
> > - Write the code by hand for each platform (e.g., see
> >   src/x64/builtins-x64.(h|cc)) and register it during bootstrap (see
> >   bootstrapper.cc, search for SimpleInstallFunction).
> > 
> > - If you are looking for adventure (or strongly dislike writing
> >   platform-specific code), you could have a go at writing the builtins
> >   in Turbofan, using code-stub-assembler.h. The infrastructure is
> >   very much work in progress, but it would be awesome if we had
> >   someone who would try to use that.
> > 
> > In both cases, you could still use the stub; you would 'only' write
> > new code for all the checks and conversions that surround the actual
> > atomic access.
> 
> OK, this sounds doable. Sure, I'll take a look at being the guinea pig^W^W^W^W
> generating the builtin from TF. :)
> 
> It sounds like it is worth going ahead and doing it in this CL?

I would be fine with you first landing the stub minus the plumbing to
various compilers (so it would be essentially dead code, which you
would wire up in a separate CL). Up to you.

> > That said, I think you could also tweak your current intrinsic
> > plumbing, but you would have to find a way to make it work with the
> > interpreter.  AFAIK, interpreter does not easily support calls to
> > arbitrary stubs.  The call itself is not a problem, but there is
> > limited number of opcodes and the encoding for intrinsics has not been
> > designed yet.
> 
> It sounds like we'll have to come up with a better solution for the
interpreter
> either way? Or are you saying that the interpreter will correctly call the
> builtin but not the intrinsic without allocating an opcode?

Yes, calls to builtins are transparent to interpreter, it just works.

> > In any case, you will want to be more clever in Turbofan. If you do
> > the builtin approach, you would not want to pay the overhead of
> > checks/conversions if you already know the types statically. With the
> > current approach based on intrinsics you would definitely want to
> > avoid the overhead of runtime call to  %IsSharedIntegerTypedArray, and
> > you would probably have to do something about the conversions - I do
> > not think Turbofan would be quite able to elide all of them even if
> > the types are statically known.
> 
> Yes, I assumed I'd have to do more work in TF. I added what's there now just
so
> TF worked at all. I figure I can work on the optimized version in another CL,
> wdyt?

Makes sense.

> > CC'ing @bmeurer to check that I am not talking nonsense, and @mcilroy
> > for the interpreter bit.

[Jarin, 2016-02-01 07:42:43]

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5447: static void
GetTypedArrayBackingStore(MacroAssembler* masm,
On 2016/01/28 16:20:00, binji wrote:
> On 2016/01/28 08:11:12, Jarin wrote:
> > Nit: Anonymous namespace for all the static methods, please.
> 
> Happy to do it, but it doesn't seem to be consistent with the rest of the
file.
> Should I change the others too?

I see your point, but I still think new code should not use statics. However,
this is just a nit, so I am happy with whatever decision you make.

https://codereview.chromium.org/1617503003/diff/60002/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5505: STATIC_ASSERT(FIXED_UINT8_CLAMPED_ARRAY_TYPE ==
FIXED_INT8_ARRAY_TYPE + 8);
On 2016/01/28 16:20:00, binji wrote:
> On 2016/01/28 08:11:12, Jarin wrote:
> > I like this. Thanks!
> 
> Yeah, it's a bit paranoid, but doesn't really hurt. :)

I actually like that it explicitly states what you rely on for documentation. It
is actually the best kind of documentation, checked documentation!

https://codereview.chromium.org/1617503003/diff/60002/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:3908: // TODO(binji): implement
On 2016/01/28 16:20:00, binji wrote:
> On 2016/01/28 08:11:12, Jarin wrote:
> > How about having a global variable for the fences? Then we would atomically
> > increment it by 0 here. This should give us strong-enough guarantees for the
> > barriers, I think.
> 
> Maybe... looking at http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html, the
> sequence for ARM we'll generate is:
> 
>   LoadSeqCst: ldr; dmb
>   StoreSeqCst: dmb; str; dmb
> 
> it looks like we could translate dmb -> mfence:
> 
>   LoadSeqCst: mov; mfence
>   StoreSeqCst: mfence; mov; mfence
> 
> This would then match the x86 SeqCst alternate implementation in that doc
(with
> extra fences):
> 
>   LoadSeqCst: mov
>   StoreSeqCst: mov; mfence
> 
> But I'm not sure the atomic increment (probably xadd 0...?) would have the
same
> guarantees. What do you think?

Atomic (i.e., locked) increment on x86s has a built-in fence. I believe this
should work on x86. I am a bit less sure about the other architectures (e.g.,
simulating arm on ppc), but I think we should still be ok.

https://codereview.chromium.org/1617503003/diff/60002/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1617503003/diff/60002/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:25: static CallDescriptor::Flags
AdjustFrameStatesForCall(Node* node) {
On 2016/01/28 16:20:01, binji wrote:
> On 2016/01/28 08:11:12, Jarin wrote:
> > I believe here you only need a simpler version because the frame state count
> > should be always > 0. 
> > 
> > Do you ever see count == 0 here?
> 
> No, I think I was ending up with multiple frame states.

What I am saying is that you should not adapt based on the frame state count of
the node. Since the load can never cause a deoptimization, it should be fine to
always remove all the frame state inputs and then set CallDescriptor::kNoFlags
for the call.

> > If no, then maybe DCHECK(count > 0) and you do not have to pass the flags in
> the
> > methods below.
> > 
> > Actually, thinking about it more, your stubs should never cause lazy deopt,
so
> > maybe we could try to wire this up without frame states. In any case, you
> should
> > know statically whether you need a frame state or not.
> 
> Can't say I fully understand this. I just found that when I tried to call the
> stub without this, I was hitting an assert due to mismatching input counts.

Yeah, it is complicated, not many people understand this. If we do the builtin
approach, none of this will be necessary.

[binji, 2016-02-03 01:58:06]

I've started experimenting with using the CodeStubAssembler and builtins. PTAL
and see if it looks reasonable so far.

I was able to get it compiling when I had the intrinsic wiring still in, but
since I've removed it I'm now getting this error during compile:


[6/10] ACTION v8_external_snapshot: run_mksnapshot
(external)_f7558c6598d362944c5134d6b24b1101
FAILED: cd ../../tools/gyp; ../../out/Debug/mksnapshot --log-snapshot-positions
--logfile ../../out/Debug/obj/tools/gyp/v8_external_snapshot.gen/snapshot.log
--random-seed 314159265 --startup_blob ../../out/Debug/snapshot_blob.bin ""


#
# Fatal error in ../../src/x64/codegen-x64.cc, line 580
# Check failed: result || isolate->code_aging_helper()->IsOld(sequence).
#


Any ideas?

[Jarin, 2016-02-03 09:04:08]

The ComputeFlags fix should address your problem.

https://codereview.chromium.org/1617503003/diff/110001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1617503003/diff/110001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2438: Builtins::kAtomicsLoadCheck, 2, false);
Here, you should pass "true" as the last argument. This would make sure that the
builtin is always called with two arguments (if the caller provides different
number of arguments, extra arguments will be ignored and missing arguments will
be undefined).

https://codereview.chromium.org/1617503003/diff/110001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/110001/src/builtins.cc#newcod...
src/builtins.cc:4179: CodeStubAssembler assembler(isolate, &zone, descriptor,
Code::STUB,
Code::Stub ==> Code::ComputeFlags(Code::STUB)

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:53: Node* StackArgOrUndefined(Node* num_args,
int value);
You should not need this, just use Parameter.

[binji, 2016-02-03 22:09:02]

https://codereview.chromium.org/1617503003/diff/110001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1617503003/diff/110001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2438: Builtins::kAtomicsLoadCheck, 2, false);
On 2016/02/03 09:04:07, Jarin wrote:
> Here, you should pass "true" as the last argument. This would make sure that
the
> builtin is always called with two arguments (if the caller provides different
> number of arguments, extra arguments will be ignored and missing arguments
will
> be undefined).

Cool, that seems to work. Thanks! I saw "adapt", but I didn't know what that
meant. :)

https://codereview.chromium.org/1617503003/diff/110001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/110001/src/builtins.cc#newcod...
src/builtins.cc:4179: CodeStubAssembler assembler(isolate, &zone, descriptor,
Code::STUB,
On 2016/02/03 09:04:07, Jarin wrote:
> Code::Stub ==> Code::ComputeFlags(Code::STUB)

Oops. Would have been nice if the compiler caught that.

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:53: Node* StackArgOrUndefined(Node* num_args,
int value);
On 2016/02/03 09:04:07, Jarin wrote:
> You should not need this, just use Parameter.

This doesn't seem to work, perhaps I'm doing it wrong? Parameter() seems to
return the descriptor registers, in order. (e.g. Parameter(0) returns the value
of rax, which is argc for the function call). But the builtin has all of its
parameters stored on the stack. Is there an easy way to automatically fill the
registers with these stack values?

[Jarin, 2016-02-04 05:28:01]

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:53: Node* StackArgOrUndefined(Node* num_args,
int value);
On 2016/02/03 22:09:02, binji wrote:
> On 2016/02/03 09:04:07, Jarin wrote:
> > You should not need this, just use Parameter.
> 
> This doesn't seem to work, perhaps I'm doing it wrong? Parameter() seems to
> return the descriptor registers, in order. (e.g. Parameter(0) returns the
value
> of rax, which is argc for the function call). But the builtin has all of its
> parameters stored on the stack. Is there an easy way to automatically fill the
> registers with these stack values?

We looked at this with Benedikt and it looks like the problem is that the
linkage uses the BuiltinDescriptor to describe the parameters, and that only
defines the register parameters. 

Instead, we need to initialize CodeStubAssembler with JSCallDesriptor. Could you
try to define a new constructor that calls Linkage::JSCallDescriptor (rather
than  Linkage::GetStubCallDescriptor)?

In any case, thanks for exploring this, you are awesome!

[binji, 2016-02-05 02:53:01]

Made some progress in builtins.cc and code-stub-assembler.{h,cc}. PTAL and see
if this looks good so far.

In particular, take a look at how I wrapped the label stuff; does this seem
reasonable? It seems like we're not gaining a lot of benefit from hiding the
RawMachineAssembler, as it feels like I'm basically just forwarding many
function calls straight through. What do you think?

Also, I'm being pretty sloppy with Word vs. Word32 vs. Int32 operators. Seems to
work, but I'm wondering how much this matters.

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1617503003/diff/110001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:53: Node* StackArgOrUndefined(Node* num_args,
int value);
On 2016/02/04 05:28:01, Jarin wrote:
> On 2016/02/03 22:09:02, binji wrote:
> > On 2016/02/03 09:04:07, Jarin wrote:
> > > You should not need this, just use Parameter.
> > 
> > This doesn't seem to work, perhaps I'm doing it wrong? Parameter() seems to
> > return the descriptor registers, in order. (e.g. Parameter(0) returns the
> value
> > of rax, which is argc for the function call). But the builtin has all of its
> > parameters stored on the stack. Is there an easy way to automatically fill
the
> > registers with these stack values?
> 
> We looked at this with Benedikt and it looks like the problem is that the
> linkage uses the BuiltinDescriptor to describe the parameters, and that only
> defines the register parameters. 
> 
> Instead, we need to initialize CodeStubAssembler with JSCallDesriptor. Could
you
> try to define a new constructor that calls Linkage::JSCallDescriptor (rather
> than  Linkage::GetStubCallDescriptor)?
> 
> In any case, thanks for exploring this, you are awesome!

Thanks, that works. Done!

[Jarin, 2016-02-08 10:25:04]

Overall direction looking good. Some comments below.

https://codereview.chromium.org/1617503003/diff/130001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/130001/src/builtins.cc#newcod...
src/builtins.cc:4225: a.SmiUntag(a.CallRuntime(Runtime::kToInteger,
a.Parameter(1)));
How do you know that ToInteger returns a Smi? You probably want
ChangeTaggedToInt32 here.

https://codereview.chromium.org/1617503003/diff/130001/src/builtins.cc#newcod...
src/builtins.cc:4228: a.SmiUntag(a.LoadObjectField(param0,
JSTypedArray::kLengthOffset));
Same here.

https://codereview.chromium.org/1617503003/diff/130001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1617503003/diff/130001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:57: delete label;
We don't normally do new/delete dances and use zones instead (so deleting is not
necessary). In this case, I am not sure why you would want heap allocation at
all.

https://codereview.chromium.org/1617503003/diff/130001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:254: RawMachineLabel* result = new
RawMachineLabel();
As noted above, we do not normally allocate on the C++ heap directly. Normally,
you should do something like 

new (zone()->New(sizeof(RawMachineLabel)) RawMachineLabel()

or you can inherit from ZoneObject (which overloads new) and then you can say

new (zone()) RawMachineLabel()

If RawMachineLabel is a field of Label, none of this is necessary.

https://codereview.chromium.org/1617503003/diff/130001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1617503003/diff/130001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:146: RawMachineLabel* raw_label_;
Why is this a pointer? Can't it be an embedded field?

[Jarin, 2016-02-08 10:32:53]

On 2016/02/08 10:25:04, Jarin wrote:
>
https://codereview.chromium.org/1617503003/diff/130001/src/compiler/code-stub...
> src/compiler/code-stub-assembler.h:146: RawMachineLabel* raw_label_;
> Why is this a pointer? Can't it be an embedded field?

Ah, I see; we would have to expose RawMachineLabel to the world, and we do not
want that. In any case, you should rebase - there is already an implementation
of Labels (which zone-allocates).

[binji, 2016-02-12 01:46:56]

The AtomicsLoad builtin mostly works now, PTAL. There are still some problems,
though; see my comments.

https://codereview.chromium.org/1617503003/diff/170001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/170001/src/builtins.cc#newcod...
src/builtins.cc:4375: a->Goto(&invalid);
This Bind/Goto dance I end up having to do a lot of times to prevent hitting the
assert at
https://chromium.googlesource.com/v8/v8/+/master/src/compiler/instruction.cc#626.

It seemed pretty random to me, probably because it is difficult to tell what
basic blocks are being generated under the hood with the CSA.

For example, I tried to make the instance type check below a Switch(), but
couldn't figure out how to make it work.

https://codereview.chromium.org/1617503003/diff/170001/src/builtins.cc#newcod...
src/builtins.cc:4572: masm->Jump(a.GenerateCode(), RelocInfo::CODE_TARGET);
Is this right? I've seen some errors when I run certain tests that are giving
strange results. For example:

=== mjsunit/harmony/futex ===                    
/usr/local/google/home/binji/dev/v8/v8/test/mjsunit/harmony/futex.js:211:
TypeError: assertEquals is not a function
    assertEquals(0, Atomics.load(i32a, waitingId));
    ^
TypeError: assertEquals is not a function

This makes me think that I'm not setting up the frame correctly or something...

https://codereview.chromium.org/1617503003/diff/170001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1617503003/diff/170001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:268: Node* CodeStubAssembler::Context() {
This seems a bit weird, but seems to work. Better way to do this?

[Benedikt Meurer, 2016-02-12 05:58:38]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-02-12 05:58:38]

https://codereview.chromium.org/1617503003/diff/170001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1617503003/diff/170001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:268: Node* CodeStubAssembler::Context() {
I think we shouldn't have this helper method, it's actually confusing. Please
just use the Parameter node in the builtin.

[binji, 2016-02-12 20:35:18]

https://codereview.chromium.org/1617503003/diff/170001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1617503003/diff/170001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:268: Node* CodeStubAssembler::Context() {
On 2016/02/12 05:58:38, Benedikt Meurer wrote:
> I think we shouldn't have this helper method, it's actually confusing. Please
> just use the Parameter node in the builtin.

Done.

[Jarin, 2016-02-15 10:30:15]

https://codereview.chromium.org/1617503003/diff/190001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/190001/src/builtins.cc#newcod...
src/builtins.cc:4560: Node* context = a.Parameter(2);
I am a bit surprised that the parameters work here. I believe index 0 is the
receiver, i.e., the Atomics object. The real arguments start with index 1, after
the arguments goes the context. (And yes, all this parameter indexing and
parameter counts are a bit of a mess).

In your case, I though the indexes should be:

Node* array = a.Parameter(1);
Node* index = a.Parameter(2);
Node* context = a.Parameter(3);

[binji, 2016-02-15 20:31:31]

https://codereview.chromium.org/1617503003/diff/190001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/190001/src/builtins.cc#newcod...
src/builtins.cc:4560: Node* context = a.Parameter(2);
On 2016/02/15 10:30:15, Jarin wrote:
> I am a bit surprised that the parameters work here. I believe index 0 is the
> receiver, i.e., the Atomics object. The real arguments start with index 1,
after
> the arguments goes the context. (And yes, all this parameter indexing and
> parameter counts are a bit of a mess).
> 
> In your case, I though the indexes should be:
> 
> Node* array = a.Parameter(1);
> Node* index = a.Parameter(2);
> Node* context = a.Parameter(3);

Yes, you're right! Thanks, that was the bug I was looking for. Looks like it
worked because I told the assembler the function only had two args. Since they
are pushed in reverse order, it was accessing the correct values with an index
of one less, but it was also leaving an extra value on the stack (which explains
why I was seeing a stack overflow in some cases as well).

[binji, 2016-04-05 22:11:40]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* Add dmb, dsb, isb instructions to ARM disassembler, simulator
* Add test for new barrier instructions
* New atomic code stubs for x64, ia32, ARM
* Wire up code stubs to FCG, CS and TF
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, ARM
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

BUG=v8:4614
==========

[Benedikt Meurer, 2016-04-06 05:51:22]

https://codereview.chromium.org/1617503003/diff/330001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/330001/src/builtins.cc#newcod...
src/builtins.cc:4851: Node* is_float32 = a->WordEqual(elements_instance_type,
You can simplify this to instance_type < FIXED_FLOAT32_ARRAY_TYPE, guarded by
STATIC_ASSERTs.

[Rodolph Perfetta, 2016-04-06 14:03:17]

rodolph.perfetta@gmail.com changed reviewers:
+ rodolph.perfetta@gmail.com

[Rodolph Perfetta, 2016-04-06 14:03:18]

DBC

https://codereview.chromium.org/1617503003/diff/330001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/330001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5583: __ UntagAndJumpIfNotSmi(value, value, &not_smi);
How about
  __ UntagAndJumpIfSmi(value, value, &done);
then you don't need the jump below

https://codereview.chromium.org/1617503003/diff/330001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5588: __ vmov(value, double_scratch.low());
You are missing a vcvt before the vmov

[binji, 2016-04-06 19:26:02]

https://codereview.chromium.org/1617503003/diff/330001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/330001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5583: __ UntagAndJumpIfNotSmi(value, value, &not_smi);
On 2016/04/06 14:03:18, Rodolph Perfetta wrote:
> How about
>   __ UntagAndJumpIfSmi(value, value, &done);
> then you don't need the jump below

I actually don't need this function anymore, so I just removed it.

https://codereview.chromium.org/1617503003/diff/330001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5588: __ vmov(value, double_scratch.low());
On 2016/04/06 14:03:18, Rodolph Perfetta wrote:
> You are missing a vcvt before the vmov

Acknowledged.

https://codereview.chromium.org/1617503003/diff/330001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1617503003/diff/330001/src/builtins.cc#newcod...
src/builtins.cc:4851: Node* is_float32 = a->WordEqual(elements_instance_type,
On 2016/04/06 05:51:21, Benedikt Meurer wrote:
> You can simplify this to instance_type < FIXED_FLOAT32_ARRAY_TYPE, guarded by
> STATIC_ASSERTs.

Done.

[binji, 2016-04-06 23:00:10]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, ARM
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

BUG=v8:4614
==========

[Benedikt Meurer, 2016-04-07 01:33:03]

Nice. LGTM from my side.

[Rodolph Perfetta, 2016-04-07 14:13:50]

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5573: __ vldr(double_scratch, scratch,
HeapNumber::kValueOffset - kHeapObjectTag);
When you arrive here, scratch has been untagged (UntagAndJumpIfNotSmi line 5566)
so it is not a valid pointer any more.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5596: __ mov(scratch2,
Operand(static_cast<uint8_t>(FIXED_INT8_ARRAY_TYPE)));
add/sub can handle any 8-bit constant directly, so remove this line, remove the
need for scratch2 and use the immediate in the sub below.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5638: __ mov(scratch, Operand(0x40000000U));
cmp can handle any 8-bit value with an even shift, so this line is not needed,
directly use the immediate in the cmp below.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5681: __ ldrb(r0, MemOperand(backing_store, index));
if you use ldrsb, then no need to use sxtb below

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5694: __ ldrh(r0, MemOperand(backing_store, index,
LSL, 1));
use ldrsh to avoid the sxth below

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5948: __ ldr(scratch, FieldMemOperand(object,
JSTypedArray::kBufferOffset));
on arm64 the convention is to use the macro assembler where all the instructions
methods start with an upper case.

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5955: __ SmiUntag(scratch);
you can combine this with the line below:

  __ Add(backing_store, backing_store, Operand::UntagSmi(scratch));

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5983: __ Mov(scratch2,
static_cast<uint8_t>(FIXED_INT8_ARRAY_TYPE));
this can be merged in the sub below

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5990: __ Mov(scratch2, scratch);
You don't need this, below simply adr into scratch2

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:6016: __ Mov(scratch, 0x40000000U);
on 64-bits platform Smi are signed 32-bits integer, so the issue is only when
the top bit is set. You can replace those three lines with:

  __ Tbnz(value, 31, &not_smi);

[binji, 2016-04-08 18:21:42]

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5573: __ vldr(double_scratch, scratch,
HeapNumber::kValueOffset - kHeapObjectTag);
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> When you arrive here, scratch has been untagged (UntagAndJumpIfNotSmi line
5566)
> so it is not a valid pointer any more.

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5596: __ mov(scratch2,
Operand(static_cast<uint8_t>(FIXED_INT8_ARRAY_TYPE)));
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> add/sub can handle any 8-bit constant directly, so remove this line, remove
the
> need for scratch2 and use the immediate in the sub below.

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5638: __ mov(scratch, Operand(0x40000000U));
On 2016/04/07 14:13:49, Rodolph Perfetta wrote:
> cmp can handle any 8-bit value with an even shift, so this line is not needed,
> directly use the immediate in the cmp below.

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5681: __ ldrb(r0, MemOperand(backing_store, index));
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> if you use ldrsb, then no need to use sxtb below

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5694: __ ldrh(r0, MemOperand(backing_store, index,
LSL, 1));
On 2016/04/07 14:13:49, Rodolph Perfetta wrote:
> use ldrsh to avoid the sxth below

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5948: __ ldr(scratch, FieldMemOperand(object,
JSTypedArray::kBufferOffset));
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> on arm64 the convention is to use the macro assembler where all the
instructions
> methods start with an upper case.

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5955: __ SmiUntag(scratch);
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> you can combine this with the line below:
> 
>   __ Add(backing_store, backing_store, Operand::UntagSmi(scratch));

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5983: __ Mov(scratch2,
static_cast<uint8_t>(FIXED_INT8_ARRAY_TYPE));
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> this can be merged in the sub below

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:5990: __ Mov(scratch2, scratch);
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> You don't need this, below simply adr into scratch2

Done.

https://codereview.chromium.org/1617503003/diff/410001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:6016: __ Mov(scratch, 0x40000000U);
On 2016/04/07 14:13:50, Rodolph Perfetta wrote:
> on 64-bits platform Smi are signed 32-bits integer, so the issue is only when
> the top bit is set. You can replace those three lines with:
> 
>   __ Tbnz(value, 31, &not_smi);

Done.

[Rodolph Perfetta, 2016-04-11 09:15:55]

arm/arm64 looks good, just a few minor nits.

https://codereview.chromium.org/1617503003/diff/430001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/430001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5568: __ SmiUntag(scratch);
this can be merged into the instruction below with Operand::SmiUntag.

https://codereview.chromium.org/1617503003/diff/430001/src/arm64/code-stubs-a...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1617503003/diff/430001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:6012: Register scratch2, Register scratch3) {
scratch3 is not used anymore

https://codereview.chromium.org/1617503003/diff/430001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:6078: __ Uxtw(x0, x0);
I missed that the first time, this is not needed as writing to a w register
always clear the top part of the underlying x register. So the ldr w0 will have
taken care of that.

[binji, 2016-04-11 17:27:33]

https://codereview.chromium.org/1617503003/diff/430001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1617503003/diff/430001/src/arm/code-stubs-arm...
src/arm/code-stubs-arm.cc:5568: __ SmiUntag(scratch);
On 2016/04/11 09:15:55, Rodolph Perfetta wrote:
> this can be merged into the instruction below with Operand::SmiUntag.

Done.

https://codereview.chromium.org/1617503003/diff/430001/src/arm64/code-stubs-a...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1617503003/diff/430001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:6012: Register scratch2, Register scratch3) {
On 2016/04/11 09:15:55, Rodolph Perfetta wrote:
> scratch3 is not used anymore

Done.

https://codereview.chromium.org/1617503003/diff/430001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:6078: __ Uxtw(x0, x0);
On 2016/04/11 09:15:55, Rodolph Perfetta wrote:
> I missed that the first time, this is not needed as writing to a w register
> always clear the top part of the underlying x register. So the ldr w0 will
have
> taken care of that.

Ah, thanks. I thought it might do that (x64 does, I believe), but I wasn't sure.

[binji, 2016-04-11 23:45:18]

The CQ bit was checked by binji@chromium.org

[binji, 2016-04-11 23:45:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1617503003/#ps490001
(title: "merge again")

[commit-bot: I haz the power, 2016-04-11 23:45:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1617503003/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1617503003/490001

[commit-bot: I haz the power, 2016-04-11 23:47:38]

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

[commit-bot: I haz the power, 2016-04-11 23:47:39]

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

[binji, 2016-04-11 23:59:46]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers
* Bump kStubMajorKeyBits to 8

BUG=v8:4614
==========

[binji, 2016-04-12 00:00:41]

FYI, had to bump kStubMajorKeyBits to 8.

[binji, 2016-04-12 00:06:42]

On 2016/04/12 00:00:41, binji wrote:
> FYI, had to bump kStubMajorKeyBits to 8.

Ooh, but that only works on 64-bit arches. :(

Can ScriptContextFieldStub::kContextIndexBits or
ScriptContextFieldStub::kSlotIndexBits be reduced?

[Benedikt Meurer, 2016-04-12 06:47:39]

On 2016/04/12 00:06:42, binji wrote:
> On 2016/04/12 00:00:41, binji wrote:
> > FYI, had to bump kStubMajorKeyBits to 8.
> 
> Ooh, but that only works on 64-bit arches. :(
> 
> Can ScriptContextFieldStub::kContextIndexBits or
> ScriptContextFieldStub::kSlotIndexBits be reduced?

I think we can do that. Or we need to migrate a few CodeStubs to builtins (those
that don't use the minor key are no different from builtins w/ special
CallInterfaceDescriptors anyway).

[binji, 2016-04-12 22:49:59]

On 2016/04/12 06:47:39, Benedikt Meurer wrote:
> On 2016/04/12 00:06:42, binji wrote:
> > On 2016/04/12 00:00:41, binji wrote:
> > > FYI, had to bump kStubMajorKeyBits to 8.
> > 
> > Ooh, but that only works on 64-bit arches. :(
> > 
> > Can ScriptContextFieldStub::kContextIndexBits or
> > ScriptContextFieldStub::kSlotIndexBits be reduced?
> 
> I think we can do that. Or we need to migrate a few CodeStubs to builtins
(those
> that don't use the minor key are no different from builtins w/ special
> CallInterfaceDescriptors anyway).

OK, I've reduced kSlotIndexBits to 12. It's passed the bots, so it should be
fine, right? :)

[binji, 2016-04-12 22:51:34]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers
* Bump kStubMajorKeyBits to 8

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
==========

[binji, 2016-04-12 22:52:53]

The CQ bit was checked by binji@chromium.org

[binji, 2016-04-12 22:52:53]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1617503003/#ps530001
(title: "merge master + reduce kSlotIndexBits")

[commit-bot: I haz the power, 2016-04-12 22:52:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1617503003/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1617503003/530001

[commit-bot: I haz the power, 2016-04-12 22:56:47]

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

[commit-bot: I haz the power, 2016-04-12 22:56:48]

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

[binji, 2016-04-12 23:02:30]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
LOG=y
==========

[binji, 2016-04-12 23:02:51]

The CQ bit was checked by binji@chromium.org

[commit-bot: I haz the power, 2016-04-12 23:03:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1617503003/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1617503003/530001

[commit-bot: I haz the power, 2016-04-12 23:10:32]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
LOG=y
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
LOG=y
==========

[commit-bot: I haz the power, 2016-04-12 23:10:34]

Committed patchset #28 (id:530001)

[commit-bot: I haz the power, 2016-04-12 23:10:46]

Description was changed from

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
LOG=y
==========

to

==========
[Atomics] code stubs for atomic operations

* New atomic code stubs for x64, ia32, arm, arm64
* Add convenience functions JumpIfNotValidSmiValue, JumpIfUintNotValidSmiValue
  to macro-assembler-ia32 (API based on x64 macro assembler)
* Remove runtime implementation of Atomics.load, the code stub should always be
  called instead
* Add new test to mjsunit atomics test; check that Smi values of different
  sizes are supported when possible, else fall back to HeapNumbers

These changes were needed to add another codestub:
* Bump kStubMajorKeyBits from 7 to 8
* Reduce ScriptContextFieldStub::kSlotIndexBits from 13 to 12

BUG=v8:4614
LOG=y

Committed: https://crrev.com/10b5febe11b318cfef130abae343183ac862e60d
Cr-Commit-Position: refs/heads/master@{#35427}
==========

[commit-bot: I haz the power, 2016-04-12 23:10:47]

Patchset 28 (id:??) landed as
https://crrev.com/10b5febe11b318cfef130abae343183ac862e60d
Cr-Commit-Position: refs/heads/master@{#35427}

[Michael Achenbach, 2016-04-13 11:58:15]

A revert of this CL (patchset #28 id:530001) has been created in
https://codereview.chromium.org/1884883003/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Roll was reverted. Please fix unused
methods, see:
https://codereview.chromium.org/1884913002/

https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds....