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-atomics.cc
File src/runtime/runtime-atomics.cc (right):
https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-atomics.cc#newcode39
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/atomics.js
File test/mjsunit/harmony/atomics.js (right):
https://codereview.chromium.org/1162503002/diff/60001/test/mjsunit/harmony/atomics.js#newcode14
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-atomics.cc
File src/runtime/runtime-atomics.cc (right):
https://codereview.chromium.org/1162503002/diff/100001/src/runtime/runtime-atomics.cc#newcode176
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.)
https://codereview.chromium.org/1162503002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
