Atomic byte operations on x86_64 need not pin any registers at all

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments)

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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #8572532 - Flags: review?(hv1989)
(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.
Attachment #8572533 - Flags: review?(hv1989)
Posted patch Test casesSplinter Review
These are for asm.js only - for plain JS there are already 8-bit tests in tests/atomics/basic-tests.js.
Attachment #8572535 - Flags: review?(hv1989)
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.
Attachment #8572532 - Flags: review?(hv1989) → review?(sunfish)
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.
Attachment #8572533 - Flags: review?(hv1989) → review+
Comment on attachment 8572535 [details] [diff] [review]
Test cases

Review of attachment 8572535 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!
Attachment #8572535 - Flags: review?(hv1989) → review+
Depends on: 1141067
Comment on attachment 8572532 [details] [diff] [review]
Bug fixes for the BaseAssembler

This patch has been moved to bug 1141067.
Attachment #8572532 - Flags: review?(sunfish)
Blocks: 1141121
https://hg.mozilla.org/mozilla-central/rev/5777a98e824f
https://hg.mozilla.org/mozilla-central/rev/9c405f41e3e7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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
Status: RESOLVED → REOPENED
Flags: needinfo?(lhansen)
Resolution: FIXED → ---
The bug is in the other patch, bug 1141067.
Flags: needinfo?(lhansen)
https://hg.mozilla.org/mozilla-central/rev/d2747e260b68
https://hg.mozilla.org/mozilla-central/rev/fbe97de16996
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.