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-atomics.cc
File src/runtime/runtime-atomics.cc (right):
https://codereview.chromium.org/1162503002/diff/60001/src/runtime/runtime-atomics.cc#newcode21
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-atomics.cc#newcode23
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-atomics.cc#newcode39
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/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/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.
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.
