Implement Atomics API

Implement Atomics API

This is behind the flag "--harmony-atomics", and it only works on
SharedArrayBuffers. This implementation only includes the runtime functions.
The TurboFan implementation will be next.

The draft spec for Atomics can be found here:
https://docs.google.com/document/d/1NDGA_gZJ7M7w1Bh8S0AoDyEqwDdRh4uSoTPSNn77PFk

BUG=
LOG=n

Committed: https://crrev.com/e59e40a3542f6611dcf4696321feea4d135169ff
Cr-Commit-Position: refs/heads/master@{#28796}

[binji, 2015-06-01 05:56:43]

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

[Jarin, 2015-06-01 06:44:05]

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

[Jarin, 2015-06-01 06:45:17]

Adding bmeurer@ to have a look at the builtin magic.

[Benedikt Meurer, 2015-06-01 06:45:23]

Can you please add "use asm" tests in mjsunit/compiler/asm as well?

[Benedikt Meurer, 2015-06-01 06:46:58]

Builtins part looks good.

[Jarin, 2015-06-01 17:50:10]

lgtm with some nits and non-blocking questions.

https://codereview.chromium.org/1162503002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1162503002/diff/60001/src/objects.h#newcode6913
src/objects.h:6913: ATOMIC_FUNCTIONS_WITH_ID_LIST(DECLARE_FUNCTION_ID)
Funny indentation.

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
File src/runtime/runtime-atomics.cc (right):

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:21: #define REQUIRE_LOCK_64_BIT 0
This should go into some header file because it has to match the compiler
(Turbofan) versions of the atomics.


In fact, the __atomic* stuff below is a bit scary because we are relying on the
C++ libraries doing the same thing as the Turbofan version. Ultimately, we might
want to create a stub or some such for the atomics to ensure consistency between
the optimized and unoptimized versions. Anyway, this does not have to be
addressed by this CL.

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:23: bool IsLockFree(uint32_t size) {
Channeling bmeurer@: The file-local methods should go into anonymous
namespace(s).

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:39: #if V8_CC_GNU
This platform-specific bifurcation is a bit ugly, perhaps we should use
atomicops.h after all (with the compare-and-swap loops for read-modify-writes
and extra barriering to get sequential consistency). The runtime functions will
be slow anyway, so the extra cost should not matter. What do you think?

https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/at...
File test/mjsunit/harmony/atomics.js (right):

https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/at...
test/mjsunit/harmony/atomics.js:14: //  {constr: Uint8ClampedArray, min: 0, max:
255},
The spec has the clamped arrays, so I guess we should support them.

[arv (Not doing code reviews), 2015-06-01 17:56:17]

Can you link to spec drafts in the CL description and in the code?

[binji, 2015-06-02 21:32:56]

https://codereview.chromium.org/1162503002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1162503002/diff/60001/src/objects.h#newcode6913
src/objects.h:6913: ATOMIC_FUNCTIONS_WITH_ID_LIST(DECLARE_FUNCTION_ID)
On 2015/06/01 at 17:50:10, jarin wrote:
> Funny indentation.

Agreed. This is what git cl format does, I think because the previous line has
no semicolon. I'll fix it for now, but I imagine this will break again when
someone else reformats the file.

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
File src/runtime/runtime-atomics.cc (right):

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:21: #define REQUIRE_LOCK_64_BIT 0
On 2015/06/01 at 17:50:10, jarin wrote:
> This should go into some header file because it has to match the compiler
(Turbofan) versions of the atomics.

Done.

> 
> 
> In fact, the __atomic* stuff below is a bit scary because we are relying on
the C++ libraries doing the same thing as the Turbofan version. Ultimately, we
might want to create a stub or some such for the atomics to ensure consistency
between the optimized and unoptimized versions. Anyway, this does not have to be
addressed by this CL.

Agreed, I think a stub probably is the right solution. As you say, I'll fix that
in a future CL.

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:23: bool IsLockFree(uint32_t size) {
On 2015/06/01 at 17:50:10, jarin wrote:
> Channeling bmeurer@: The file-local methods should go into anonymous
namespace(s).

Done.

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:39: #if V8_CC_GNU
On 2015/06/01 at 17:50:10, jarin wrote:
> This platform-specific bifurcation is a bit ugly, perhaps we should use
atomicops.h after all (with the compare-and-swap loops for read-modify-writes
and extra barriering to get sequential consistency). The runtime functions will
be slow anyway, so the extra cost should not matter. What do you think?

Well, if we end up switching to stubs anyway, it doesn't really matter. But just
in case it made the code nicer, I tried writing it up. It gets pretty nasty to
handle the masking properly for non-word sizes, e.g.

inline int8_t CompareExchangeSeqCst(int8_t* p, int8_t oldval, int8_t newval) {
  uintptr_t pint = reinterpret_cast<uintptr_t>(p);
  int shift = (pint & 3) * 8;
  base::Atomic32 mask = ~(0xff << shift);
  base::Atomic32* p32 = reinterpret_cast<base::Atomic32*>(pint & ~3);
  base::Atomic32 oldval32, result32;
  base::Atomic32 oldval_or =
      static_cast<base::Atomic32>(static_cast<int32_t>(oldval)) << shift;
  base::Atomic32 newval_or =
      static_cast<base::Atomic32>(static_cast<int32_t>(newval)) << shift;
  base::MemoryBarrier();
  do {
    oldval32 = (*p32 & mask) | oldval_or;
    base::MemoryBarrier();
    result32 = base::NoBarrier_CompareAndSwap(p32, oldval32,
                                              (oldval32 & mask) | newval_or);
    base::MemoryBarrier();
  } while ((result32 & mask) != (oldval32 & mask));
  return (result32 & ~mask) >> shift;
}

I'm not sure that's 100% correct, either. :-)

https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/at...
File test/mjsunit/harmony/atomics.js (right):

https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/at...
test/mjsunit/harmony/atomics.js:14: //  {constr: Uint8ClampedArray, min: 0, max:
255},
On 2015/06/01 at 17:50:10, jarin wrote:
> The spec has the clamped arrays, so I guess we should support them.

JF suggested we should push back on this. I agree, as it makes the code more
complex and it is not clear yet what the benefit is.

[Benedikt Meurer, 2015-06-03 03:08:14]

LGTM, although I'm still scared by several atomic operations on float32 and
float64...

[Jarin, 2015-06-03 13:30:15]

still lgtm with some non-blocking nits.

https://codereview.chromium.org/1162503002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1162503002/diff/60001/src/objects.h#newcode6913
src/objects.h:6913: ATOMIC_FUNCTIONS_WITH_ID_LIST(DECLARE_FUNCTION_ID)
On 2015/06/02 21:32:55, binji wrote:
> On 2015/06/01 at 17:50:10, jarin wrote:
> > Funny indentation.
> 
> Agreed. This is what git cl format does, I think because the previous line has
> no semicolon. I'll fix it for now, but I imagine this will break again when
> someone else reformats the file.

I think this is because  git cl format only formats lines that you touched. You
could try to touch the surrounding lines (e.g., add some whitespace) and do git
cl format again.

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
File src/runtime/runtime-atomics.cc (right):

https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-ato...
src/runtime/runtime-atomics.cc:39: #if V8_CC_GNU
On 2015/06/02 21:32:55, binji wrote:
> On 2015/06/01 at 17:50:10, jarin wrote:
> > This platform-specific bifurcation is a bit ugly, perhaps we should use
> atomicops.h after all (with the compare-and-swap loops for read-modify-writes
> and extra barriering to get sequential consistency). The runtime functions
will
> be slow anyway, so the extra cost should not matter. What do you think?
> 
> Well, if we end up switching to stubs anyway, it doesn't really matter. But
just
> in case it made the code nicer, I tried writing it up. It gets pretty nasty to
> handle the masking properly for non-word sizes, e.g.
> 
> inline int8_t CompareExchangeSeqCst(int8_t* p, int8_t oldval, int8_t newval) {
>   uintptr_t pint = reinterpret_cast<uintptr_t>(p);
>   int shift = (pint & 3) * 8;
>   base::Atomic32 mask = ~(0xff << shift);
>   base::Atomic32* p32 = reinterpret_cast<base::Atomic32*>(pint & ~3);
>   base::Atomic32 oldval32, result32;
>   base::Atomic32 oldval_or =
>       static_cast<base::Atomic32>(static_cast<int32_t>(oldval)) << shift;
>   base::Atomic32 newval_or =
>       static_cast<base::Atomic32>(static_cast<int32_t>(newval)) << shift;
>   base::MemoryBarrier();
>   do {
>     oldval32 = (*p32 & mask) | oldval_or;
>     base::MemoryBarrier();
>     result32 = base::NoBarrier_CompareAndSwap(p32, oldval32,
>                                               (oldval32 & mask) | newval_or);
>     base::MemoryBarrier();
>   } while ((result32 & mask) != (oldval32 & mask));
>   return (result32 & ~mask) >> shift;
> }
> 
> I'm not sure that's 100% correct, either. :-)

Yeah, that is a bit nasty. Maybe it is best to leave as is for now, I'll leave
it up to you.

https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/at...
File test/mjsunit/harmony/atomics.js (right):

https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/at...
test/mjsunit/harmony/atomics.js:14: //  {constr: Uint8ClampedArray, min: 0, max:
255},
On 2015/06/02 21:32:55, binji wrote:
> On 2015/06/01 at 17:50:10, jarin wrote:
> > The spec has the clamped arrays, so I guess we should support them.
> 
> JF suggested we should push back on this. I agree, as it makes the code more
> complex and it is not clear yet what the benefit is.

Acknowledged.

https://codereview.chromium.org/1162503002/diff/100001/src/runtime/runtime-at...
File src/runtime/runtime-atomics.cc (right):

https://codereview.chromium.org/1162503002/diff/100001/src/runtime/runtime-at...
src/runtime/runtime-atomics.cc:176: }  // anonymous namespace
Could not even the stuff below go into the anonymous namespace? (Basically
everything except the runtime functions.)

[binji, 2015-06-03 16:39:45]

The CQ bit was checked by binji@chromium.org

[binji, 2015-06-03 16:39:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1162503002/#ps120001
(title: "add more symbols to anonymous namespace")

[commit-bot: I haz the power, 2015-06-03 16:40:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162503002/120001

[commit-bot: I haz the power, 2015-06-03 17:58:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-03 17:58:54]

Patchset 7 (id:??) landed as
https://crrev.com/e59e40a3542f6611dcf4696321feea4d135169ff
Cr-Commit-Position: refs/heads/master@{#28796}

[binji, 2015-06-03 20:16:32]

https://codereview.chromium.org/1162503002/diff/100001/src/runtime/runtime-at...
File src/runtime/runtime-atomics.cc (right):

https://codereview.chromium.org/1162503002/diff/100001/src/runtime/runtime-at...
src/runtime/runtime-atomics.cc:176: }  // anonymous namespace
On 2015/06/03 at 13:30:15, jarin wrote:
> Could not even the stuff below go into the anonymous namespace? (Basically
everything except the runtime functions.)

Done.