[INFER] Make TypeInference work on solaris sparc

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Leon Sha, Assigned: Leon Sha)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-jaegermonkey)

Attachments

(1 attachment, 2 obsolete attachments)

25.30 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
TypeInference is not working on solaris sparc.
(Assignee)

Comment 1

7 years ago
Created attachment 548393 [details] [diff] [review]
patch
Assignee: general → leon.sha
Status: NEW → ASSIGNED
Attachment #548393 - Flags: review?(bhackett1024)
Comment on attachment 548393 [details] [diff] [review]
patch

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

::: js/src/methodjit/BaseAssembler.h
@@ +321,5 @@
>          return JS_FUNC_TO_DATA_PTR(void *, JaegerStubVeneer);
> +#elif defined(JS_CPU_SPARC)
> +        /*
> +         * We can simulate the situation in jited code to let call return to a
> +         * target address loacated on stack without veneer. We record the return

Typo

::: js/src/methodjit/Compiler.cpp
@@ +4151,5 @@
>              RegisterID reg = frame.tempRegForData(top);
>              frame.pop();
> +            RegisterID dataReg = frame.allocReg();
> +            masm.loadPtr(Address(reg, offsetof(JSObject, privateData)), dataReg);
> +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);

Why is this change necessary?

@@ +4174,5 @@
>              masm.loadPtr(Address(reg, offsetof(JSObject, privateData)), reg);
>              frame.pop();
> +            RegisterID dataReg = frame.allocReg();
> +            masm.loadPtr(Address(reg, TypedArray::lengthOffset()), dataReg);
> +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);

Ditto

@@ +4190,5 @@
>          if (types->isLazyArguments(cx)) {
>              frame.pop();
> +            RegisterID dataReg = frame.allocReg();
> +            masm.loadPtr(Address(JSFrameReg, StackFrame::offsetOfArgs()), dataReg);
> +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);

Ditto

::: js/src/methodjit/ImmutableSync.cpp
@@ +93,5 @@
>          RegisterID reg = RegisterID(i);
>          if (!(Registers::maskReg(reg) & Registers::AvailRegs))
>              continue;
>  
> +        lastResort = i;

Ditto

::: js/src/methodjit/LoopState.cpp
@@ +1349,5 @@
>              masm.loadPtr(Address(T0, offset), T0);
>              if (entry.kind == InvariantEntry::INVARIANT_LENGTH)
>                  masm.storeValueFromComponents(ImmType(JSVAL_TYPE_INT32), T0, address);
>              else
> +                masm.storePayload(T0, address);

This is storing the slot pointer from an object, not a Value payload, so storing it as a payload will require extra memory traffic on x64.  Why is this change necessary?

@@ +1356,5 @@
>  
>            case InvariantEntry::INVARIANT_ARGS_BASE: {
>              Address address = frame.addressOf(frame.getTemporary(entry.u.array.temporary));
>              masm.loadFrameActuals(outerScript->fun, T0);
> +            masm.storePayload(T0, address);

Ditto
Attachment #548393 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Comment on attachment 548393 [details] [diff] [review] [review]
> patch
> 
> Review of attachment 548393 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/methodjit/BaseAssembler.h
> @@ +321,5 @@
> >          return JS_FUNC_TO_DATA_PTR(void *, JaegerStubVeneer);
> > +#elif defined(JS_CPU_SPARC)
> > +        /*
> > +         * We can simulate the situation in jited code to let call return to a
> > +         * target address loacated on stack without veneer. We record the return
> 
> Typo
> 
> ::: js/src/methodjit/Compiler.cpp
> @@ +4151,5 @@
> >              RegisterID reg = frame.tempRegForData(top);
> >              frame.pop();
> > +            RegisterID dataReg = frame.allocReg();
> > +            masm.loadPtr(Address(reg, offsetof(JSObject, privateData)), dataReg);
> > +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);
> 
> Why is this change necessary?

frame.push will load the payload of address and push. The address should be a Value address. Here we want to load an object not a Value payload.

> 
> @@ +4174,5 @@
> >              masm.loadPtr(Address(reg, offsetof(JSObject, privateData)), reg);
> >              frame.pop();
> > +            RegisterID dataReg = frame.allocReg();
> > +            masm.loadPtr(Address(reg, TypedArray::lengthOffset()), dataReg);
> > +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);
> 
> Ditto
> 
> @@ +4190,5 @@
> >          if (types->isLazyArguments(cx)) {
> >              frame.pop();
> > +            RegisterID dataReg = frame.allocReg();
> > +            masm.loadPtr(Address(JSFrameReg, StackFrame::offsetOfArgs()), dataReg);
> > +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);
> 
> Ditto
> 
> ::: js/src/methodjit/ImmutableSync.cpp
> @@ +93,5 @@
> >          RegisterID reg = RegisterID(i);
> >          if (!(Registers::maskReg(reg) & Registers::AvailRegs))
> >              continue;
> >  
> > +        lastResort = i;
> 
> Ditto

At least the lastResort register should be an available register.

> 
> ::: js/src/methodjit/LoopState.cpp
> @@ +1349,5 @@
> >              masm.loadPtr(Address(T0, offset), T0);
> >              if (entry.kind == InvariantEntry::INVARIANT_LENGTH)
> >                  masm.storeValueFromComponents(ImmType(JSVAL_TYPE_INT32), T0, address);
> >              else
> > +                masm.storePayload(T0, address);
> 
> This is storing the slot pointer from an object, not a Value payload, so
> storing it as a payload will require extra memory traffic on x64.  Why is
> this change necessary?

Yes, this is storing the slot pointer from an object. But it is storing to a Value address. When the object to be loaded, loadPayload will be used.
Because of the Endian difference, on sparc Payload address is not the same as Value address. That's why we have to make store and load consistent.

> 
> @@ +1356,5 @@
> >  
> >            case InvariantEntry::INVARIANT_ARGS_BASE: {
> >              Address address = frame.addressOf(frame.getTemporary(entry.u.array.temporary));
> >              masm.loadFrameActuals(outerScript->fun, T0);
> > +            masm.storePayload(T0, address);
> 
> Ditto
(Assignee)

Comment 4

7 years ago
Created attachment 551976 [details] [diff] [review]
patch v2
Attachment #548393 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #551976 - Flags: review?(bhackett1024)
Comment on attachment 551976 [details] [diff] [review]
patch v2

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

Sorry for the delay.  r+, but it would be good if you could make the changes below.

::: js/src/methodjit/Compiler.cpp
@@ +4237,5 @@
>              RegisterID reg = frame.tempRegForData(top);
>              frame.pop();
> +            RegisterID dataReg = frame.allocReg();
> +            masm.loadPtr(Address(reg, offsetof(JSObject, privateData)), dataReg);
> +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);

Can you wrap this into a frame.pushWord method, which takes an address and loads/pushes an unboxed word of a given non-double type.

@@ +4259,5 @@
>              RegisterID reg = frame.copyDataIntoReg(top);
>              frame.pop();
> +            RegisterID dataReg = frame.allocReg();
> +            masm.loadPtr(Address(reg, TypedArray::lengthOffset()), dataReg);
> +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);

Can use above pushWord method here.

@@ +4275,5 @@
>          if (types->isLazyArguments(cx)) {
>              frame.pop();
> +            RegisterID dataReg = frame.allocReg();
> +            masm.loadPtr(Address(JSFrameReg, StackFrame::offsetOfArgs()), dataReg);
> +            frame.pushTypedPayload(JSVAL_TYPE_INT32, dataReg);

Ditto
Attachment #551976 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

7 years ago
Created attachment 552594 [details] [diff] [review]
patch v3

Added pushWord function. Add some changes according the latest jaegermonkey check-ins. I change the TypedArray::lengthOffset from offset of Value to offset of Value payload. Otherwise, we may need to handle length as Value everywhere.
(Assignee)

Updated

7 years ago
Attachment #552594 - Flags: review?(bhackett1024)
Attachment #552594 - Flags: review?(bhackett1024) → review+
Attachment #551976 - Attachment is obsolete: true
(Assignee)

Comment 7

7 years ago
http://hg.mozilla.org/projects/jaegermonkey/rev/9cea788e8c07
Whiteboard: fixed-in-jaegermonkey
Landed on m-c.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.