On x86_64 we can generate byte-sized operations on any GPR using a REX prefix, and avoid pinning any register.

Not quite that easy: ESI, EDI, EBP cannot be used as byte registers with a REX prefix, so must be excluded from the allocation.

(In reply to Lars T Hansen [:lth] from comment #1) > Not quite that easy: ESI, EDI, EBP cannot be used as byte registers with a > REX prefix, so must be excluded from the allocation. I take that back, the manual does not state this clearly but in 64-bit mode with REX.B these do indeed have distinguished byte registers that do not overlap with RAX..RDX, ie, the forms do not reference AH, BH, CH, DH. Table 3.1, Register Codes, in the Intel manual volume 2.

8-byte operations should use code generation helpers that ensure the use of proper REX prefixes on x64. These bugs were benign because the locked-instruction emitters were only used by the Atomics implementation, which pinned its registers to al/bl/cl/dl.

(In reply to Lars T Hansen [:lth] from comment #3) > Created attachment 8572532 [details] [diff] [review] > Bug fixes for the BaseAssembler > > 8-byte operations ... Sigh. 1-byte / 8-bit.

These are for asm.js only - for plain JS there are already 8-bit tests in tests/atomics/basic-tests.js.

Comment on attachment 8572532 [details] [diff] [review] Bug fixes for the BaseAssembler Review of attachment 8572532 [details] [diff] [review]: ----------------------------------------------------------------- I'm gonna forward this to sunfish. He was busy with REX, so I want him to confirm this isn't breaking his efforts.

Comment on attachment 8572533 [details] [diff] [review] Specialize x64 code generation Review of attachment 8572533 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/x64/Lowering-x64.cpp @@ +133,5 @@ > > void > +LIRGeneratorX64::visitCompareExchangeTypedArrayElement(MCompareExchangeTypedArrayElement *ins) > +{ > + lowerCompareExchangeTypedArrayElement(ins, false); lowerCompareExchangeTypedArrayElement(ins, /* useI386ByteRegisters = */ false); @@ +139,5 @@ > + > +void > +LIRGeneratorX64::visitAtomicTypedArrayElementBinop(MAtomicTypedArrayElementBinop *ins) > +{ > + lowerAtomicTypedArrayElementBinop(ins, false); lowerAtomicTypedArrayElementBinop(ins, /* useI386ByteRegisters = */ false); ::: js/src/jit/x86/Lowering-x86.cpp @@ +300,5 @@ > + case Scalar::Uint32: > + break; > + default: > + MOZ_CRASH("Unexpected array type"); > + } Can you change this in: bool byteArray = byteSize(ins->accessType()) == 1; or add a function in jsfriendapi.h::Scalar ? sizeIsByte(Scalar::Type) @@ +343,5 @@ > + case Scalar::Uint32: > + break; > + default: > + MOZ_CRASH("Unexpected array type"); > + } Same as above.

Comment on attachment 8572535 [details] [diff] [review] Test cases Review of attachment 8572535 [details] [diff] [review]: ----------------------------------------------------------------- Awesome!

Comment on attachment 8572532 [details] [diff] [review] Bug fixes for the BaseAssembler This patch has been moved to bug 1141067.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5777a98e824f https://hg.mozilla.org/integration/mozilla-inbound/rev/9c405f41e3e7

https://hg.mozilla.org/mozilla-central/rev/5777a98e824f https://hg.mozilla.org/mozilla-central/rev/9c405f41e3e7

Both backed out for making windows ggc builds permafail: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e261a74f350 https://treeherder.mozilla.org/logviewer.html#?job_id=7433213&repo=mozilla-inbound

The bug is in the other patch, bug 1141067.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2747e260b68 https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe97de16996

https://hg.mozilla.org/mozilla-central/rev/d2747e260b68 https://hg.mozilla.org/mozilla-central/rev/fbe97de16996