Closed Bug 674179 Opened 8 years ago Closed 7 years ago

[INFER] Make TypeInference work on solaris sparc

Categories

(Core :: JavaScript Engine, defect)

Sun
Solaris
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: leon.sha, Assigned: leon.sha)

Details

(Whiteboard: fixed-in-jaegermonkey)

Attachments

(1 file, 2 obsolete files)

TypeInference is not working on solaris sparc.
Attached patch patch (obsolete) — Splinter Review
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)
(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
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #548393 - Attachment is obsolete: true
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+
Attached patch patch v3Splinter Review
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.
Attachment #552594 - Flags: review?(bhackett1024)
Attachment #552594 - Flags: review?(bhackett1024) → review+
Attachment #551976 - Attachment is obsolete: true
http://hg.mozilla.org/projects/jaegermonkey/rev/9cea788e8c07
Whiteboard: fixed-in-jaegermonkey
Landed on m-c.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.