674179 - [INFER] Make TypeInference work on solaris sparc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Leon Sha]

TypeInference is not working on solaris sparc.

Comment 1

[Leon Sha]

Attachment: patch

Comment 2

[Brian Hackett [Laid off!]]

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

Comment 3

[Leon Sha]

(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

Comment 4

[Leon Sha]

Attachment: patch v2

Comment 5

[Brian Hackett [Laid off!]]

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

Comment 6

[Leon Sha]

Attachment: 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.

Comment 7

[Leon Sha]

http://hg.mozilla.org/projects/jaegermonkey/rev/9cea788e8c07

Comment 8

[Sean Stangl [:sstangl]]

Landed on m-c.