Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 1 obsolete attachment)

This bug carries forward the operand API patch under discussion in bug 916167 and accompanies it with a few related patches.
Building on bug 915833, this bug factors out x86's and x64's Operand classes into a single class shared by both.
Assignee: general → sunfish
Attachment #806821 - Flags: review?(jdemooij)
This is the main patch carried from bug 916167 with additional updates for x86 and ARM. It also takes some inspiration from bug 916912, simplifying the set of Operand constructors even further.

Additional highlights include:

 - Interfaces that copy data in and out of Operands are called "move" instead of "load" or "store".
 - The only way to construct an Operand which implies a memory dereference is to first construct an Address, BaseIndex, or AbsoluteAddress, so that the Operand(reg) vs Operand(reg, 0) confusion is avoided.
 - Many APIs can now take Addresses directly, avoiding the need for an Operand instance in many cases.
 - It fixes the problem in which movsd/movss don't actually emit movsd/movss instructions from bug 916167.

It's still a little rough around the edges, and possibly should be split up into multiple patches, but I'm hoping to get some feedback on the approach first.
A subsequent patch here makes use of MacroAssembler APIs which use ScratchReg in places where these registers are live. This patch moves them out of the way.
Attachment #806828 - Flags: review?(luke)
Comment on attachment 806828 [details] [diff] [review]
operand-fix-reg-conflict.patch

Oops!
Attachment #806828 - Flags: review?(luke) → review+
This patch updates a bunch of miscellaneous places to use higher-level MacroAssembler interfaces, which in some cases enables it to emit more efficient code, for example by enabling the use of more immediate addresses as in bug 915833.

One tricky aspect of it is that these MacroAssembler interfaces use the ScratchReg, so, among other things, this depends on the patch above to avoid conflicting with exisitng uses of that register.
Attachment #806829 - Flags: review?(luke)
Attachment #806829 - Flags: review?(luke) → review+
Comment on attachment 806821 [details] [diff] [review]
operand-shared-code.patch

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

Nice, thanks.
Attachment #806821 - Flags: review?(jdemooij) → review+
This patch changes code to use "load" and "store" MacroAssembler interfaces in place of "move" interfaces, where appropriate.
Attachment #808943 - Flags: review?(jdemooij)
This patch changes a bunch of movsd/movss calls to use loadDouble/storeDouble, in preparation for cleaning up the movsd/movss interface.
Attachment #808945 - Flags: review?(jdemooij)
This is a minor cleanup; it uses Addresses instead of Operands in a few places where the full Operand generality is unneeded.
Attachment #808947 - Flags: review?(jdemooij)
This patch removes the Operand constructors which create memory operands implicitly. It makes some things more verbose, but it introduces a useful invariant, that an Address/BaseIndex/AbsoluteAddress is needed when talking to memory, that an interface using an Operand won't implicitly use memory without one. This is one of the core ideas of the earlier dependence-breaking-reorg.patch, but it's split out, without all the extra changes in that patch.
Attachment #806824 - Attachment is obsolete: true
Attachment #808949 - Flags: review?(jdemooij)
This patch implements the use of movapd/movaps for register-to-register moves without using the movsd/movss interfaces, as discussed earlier.
Attachment #808950 - Flags: review?(jdemooij)
As a follow-up to the earlier operand-misc.patch, this patch updates several more places to use higher-level MacroAssembler APIs.
Attachment #808953 - Flags: review?(luke)
Add a comment about an interesting situation.
Attachment #808959 - Flags: review?(luke)
Attachment #808953 - Flags: review?(luke) → review+
Comment on attachment 808959 [details] [diff] [review]
operand-coupling.patch

How was that comment not there originally...
Attachment #808959 - Flags: review?(luke) → review+
Comment on attachment 808943 [details] [diff] [review]
operand-cleanup.patch

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

Beautiful, r=me with comments addressed.

Should we fix ARM as well?

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +228,2 @@
>      if (ins->arg()->isConstant()) {
> +        masm.storePtr(ImmWord(ToInt32(ins->arg())), dst);

store32(Imm32(...

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +167,5 @@
>  
>      if (load->mir()->type() == MIRType_Double)
>          masm.loadInt32OrDouble(Operand(base, offset), ToFloatRegister(load->output()));
>      else
> +        masm.loadPtr(Address(base, offset + NUNBOX32_PAYLOAD_OFFSET), ToRegister(load->output()));

It doesn't matter on x86 but I'd prefer load32 here.

@@ +803,5 @@
>          static const uint32_t TOO_BIG_EXPONENT = (DoubleExponentBias + 63) << EXPONENT_SHIFT;
>  
>          // Check exponent to avoid fp exceptions.
>          Label failPopDouble;
> +        masm.loadPtr(Address(esp, 4), output);

And here.

@@ +812,5 @@
>          masm.fld(Operand(esp, 0));
>          masm.fisttp(Operand(esp, 0));
>  
>          // Load low word, pop double and jump back.
> +        masm.loadPtr(Address(esp, 0), output);

And here.
Attachment #808943 - Flags: review?(jdemooij) → review+
Comment on attachment 808945 [details] [diff] [review]
operand-movsd-movss.patch

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

Nice how these names make it clear whether we're doing a load or a store without looking at the arguments.
Attachment #808945 - Flags: review?(jdemooij) → review+
Attachment #808947 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #20)
> Comment on attachment 808943 [details] [diff] [review]
> operand-cleanup.patch
> 
> Review of attachment 808943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Beautiful, r=me with comments addressed.
> 
> Should we fix ARM as well?

I didn't see the same use of movePtr and friends for loading and storing in the arm code.

> ::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
> @@ +228,2 @@
> >      if (ins->arg()->isConstant()) {
> > +        masm.storePtr(ImmWord(ToInt32(ins->arg())), dst);
> 
> store32(Imm32(...

Done.

> ::: js/src/jit/x86/CodeGenerator-x86.cpp
> @@ +167,5 @@
> >  
> >      if (load->mir()->type() == MIRType_Double)
> >          masm.loadInt32OrDouble(Operand(base, offset), ToFloatRegister(load->output()));
> >      else
> > +        masm.loadPtr(Address(base, offset + NUNBOX32_PAYLOAD_OFFSET), ToRegister(load->output()));
> 
> It doesn't matter on x86 but I'd prefer load32 here.

Ok.

> @@ +803,5 @@
> >          static const uint32_t TOO_BIG_EXPONENT = (DoubleExponentBias + 63) << EXPONENT_SHIFT;
> >  
> >          // Check exponent to avoid fp exceptions.
> >          Label failPopDouble;
> > +        masm.loadPtr(Address(esp, 4), output);
> 
> And here.

Ok.

> @@ +812,5 @@
> >          masm.fld(Operand(esp, 0));
> >          masm.fisttp(Operand(esp, 0));
> >  
> >          // Load low word, pop double and jump back.
> > +        masm.loadPtr(Address(esp, 0), output);
> 
> And here.

Done.

operand-cleanup.patch, operand-movsd-movss.patch, and operand-move-emitter.patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a91844d21198
https://hg.mozilla.org/integration/mozilla-inbound/rev/230cc5fecdef
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ddce77f2e7e
Comment on attachment 808949 [details] [diff] [review]
operand-address-ctors.patch

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

This patch is fine but I'm a bit worried about x86/x64 diverging more from ARM. Can we make the same changes to ARM's Operand?
Attachment #808949 - Flags: review?(jdemooij)
Comment on attachment 808950 [details] [diff] [review]
operand-movapd-movaps.patch

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

Sorry for the delay.

::: js/src/jit/shared/MoveEmitter-x86-shared.cpp
@@ +292,5 @@
>  void
>  MoveEmitterX86::emitDoubleMove(const MoveOperand &from, const MoveOperand &to)
>  {
>      if (from.isFloatReg()) {
> +        if (to.isFloatReg()) {

Nit: single-line if/else bodies so no { }
Attachment #808950 - Flags: review?(jdemooij) → review+
Posted patch fix-store-32Splinter Review
The operand-cleanup.patch accidentally changes a pointer-sized store to a 32-bit-store.  On x64, we rely on the high dword of a MIRType_Int32 to be zerod which allows use of MIRType_Int32 values in 64-bit contexts w/o a 'mov eax,eax'.  The ISA mostly does this for us.  The tricky part is when we load an int32 from the stack with a 64-bit load which apparently we do for definedFixed(LArgument(INT_ARGUMENT)).  Because of this, we need to make sure that when we store to the stack we use a full 64-bit store.

This patch reverts to the previous behavior (which fixes a WMW crash in Nightly that I just bisected to this patch).  A better follow-up fix might be to change the register allocator to emit 32-bit loads for INT_ARGUMENT.
Attachment #810472 - Flags: review?(jdemooij)
Comment on attachment 810472 [details] [diff] [review]
fix-store-32

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

Oops, I suggested that change. Turns out mov(Imm32, Operand) calls movq, not movl on 64-bit.
Attachment #810472 - Flags: review?(jdemooij) → review+
(Assuming not [leave open] anymore?)
Whiteboard: [leave open]
Thanks for the fix. Still [leave open] as there are a few patches remaining here.
Whiteboard: [leave open]
Here's operand-movapd-movaps.patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3cb2271302

With this, the only remaining patch is operand-address-ctors.patch. Given the feedback in comment 24, I looked more closely at ARM, and it appears there are non-trivial differences in how the Operator class works. It will take more investigation. For now, I'll drop this patch from this bug, since the main purpose of this bug is fixed -- the movsd and movss interfaces now just map to movsd and movss instructions.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/ba3cb2271302
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.