1020359 - IonMonkey MIPS: Fix build issues for MIPS caused by latest updates

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

MIPS build fails due to latest changes.

Some of these changes origin from bug 1017539.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: 01-emitLoadElementT.patch

Comment 2

[Branislav Rankov [:rankov]]

Attachment: 02-Fix-StoreSlotT.patch

Comment 3

[Branislav Rankov [:rankov]]

Attachment: 03-Missing.patch

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8435016 [details] [diff] [review]
02-Fix-StoreSlotT.patch

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

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +2703,5 @@
>  
> +template <typename T>
> +void
> +MacroAssemblerMIPSCompat::storeUnboxedValue(ConstantOrRegister value, MIRType valueType, const T &dest,
> +                                           MIRType slotType)

Nit: add an extra space here so that the arguments line up.

@@ +2723,5 @@
> +}
> +
> +template void
> +MacroAssemblerMIPSCompat::storeUnboxedValue(ConstantOrRegister value, MIRType valueType, const Address &dest,
> +                                           MIRType slotType);

Same here and below.

Comment 5

[Branislav Rankov [:rankov]]

Attachment: 02-Fix-StoreSlotT.patch

Fixed the nits.
Carry review from previous patch.

Comment 6

[Branislav Rankov [:rankov]]

Here is the try link for the patches:
https://tbpl.mozilla.org/?tree=Try&rev=2767a212b2be

Comment 7

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc51e7f8e8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e034ef0b7b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f441afd30e52

Comment 8

[Branislav Rankov [:rankov]]

Attachment: 04-missing-or32.patch

Comment 9

[Branislav Rankov [:rankov]]

Attachment: 05-pjs-bailout.patch

This is follow-up on bug 1020359.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4dc51e7f8e8d
https://hg.mozilla.org/mozilla-central/rev/0e034ef0b7b9
https://hg.mozilla.org/mozilla-central/rev/f441afd30e52

Comment 11

[Nicolas B. Pierron [:nbp] {back on 17/11}]

Comment on attachment 8449513 [details] [diff] [review]
05-pjs-bailout.patch

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

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +523,5 @@
>      return code;
>  }
>  
> +// NOTE: Members snapshotOffset_ and padding_ of BailoutStack
> +// are not stored in this function.

nit: "this function" ?

@@ +630,5 @@
> +    PushBailoutFrame(masm, frameClass, a0);
> +
> +    // Parallel bailout is like parallel failure in that we unwind all the way
> +    // to the entry frame. Reserve space for the frame pointer of the entry frame.
> +    const int sizeOfEntryFramePointer = sizeof(uint8_t *) * 2;

shu: Why "* 2" if this is a pointer?

Comment 12

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> Comment on attachment 8449513 [details] [diff] [review]
> 05-pjs-bailout.patch
> 
> Review of attachment 8449513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips/Trampoline-mips.cpp
> @@ +523,5 @@
> >      return code;
> >  }
> >  
> > +// NOTE: Members snapshotOffset_ and padding_ of BailoutStack
> > +// are not stored in this function.
> 
> nit: "this function" ?
> 
> @@ +630,5 @@
> > +    PushBailoutFrame(masm, frameClass, a0);
> > +
> > +    // Parallel bailout is like parallel failure in that we unwind all the way
> > +    // to the entry frame. Reserve space for the frame pointer of the entry frame.
> > +    const int sizeOfEntryFramePointer = sizeof(uint8_t *) * 2;
> 
> shu: Why "* 2" if this is a pointer?

I confess this is blindly copied from things like [1], which is also an outparam pointer. I figured it was for alignment reasons, but I don't know for sure.

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips/Trampoline-mips.cpp#548

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8449513 [details] [diff] [review]
05-pjs-bailout.patch

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

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +639,5 @@
> +    masm.passABIArg(a0);
> +    masm.passABIArg(a1);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, BailoutPar));
> +
> +    // Get the frame pointer of the entry fram and return.

Existing typo: frame. Do you mind fixing this in Trampoline-arm.cpp as well?

Comment 14

[Branislav Rankov [:rankov]]

(In reply to Shu-yu Guo [:shu] from comment #12)
> I confess this is blindly copied from things like [1], which is also an
> outparam pointer. I figured it was for alignment reasons, but I don't know
> for sure.
> 
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips/Trampoline-
> mips.cpp#548

You are right. The size of bailoutInfoOutParamSize is 2*sizeof(uintptr_t) because of alignment.

(In reply to Shu-yu Guo [:shu] from comment #13)
> ::: js/src/jit/mips/Trampoline-mips.cpp
> @@ +639,5 @@
> > +    masm.passABIArg(a0);
> > +    masm.passABIArg(a1);
> > +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, BailoutPar));
> > +
> > +    // Get the frame pointer of the entry fram and return.
> 
> Existing typo: frame. Do you mind fixing this in Trampoline-arm.cpp as well?

Sure. I'll make another patch for this.

Comment 15

[Branislav Rankov [:rankov]]

Attachment: 05-pjs-bailout.patch

Fixed nits. Carry review from previous patch.

Comment 16

[Branislav Rankov [:rankov]]

Attachment: 06-arm-typo.patch

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8450946 [details] [diff] [review]
06-arm-typo.patch

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

Thanks!

Comment 18

[Branislav Rankov [:rankov]]

https://tbpl.mozilla.org/?tree=Try&rev=1b62b18ec9fa

https://hg.mozilla.org/integration/mozilla-inbound/rev/41c2396b3761
https://hg.mozilla.org/integration/mozilla-inbound/rev/f221afb93d0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4baddb2df1db

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/41c2396b3761
https://hg.mozilla.org/mozilla-central/rev/f221afb93d0b
https://hg.mozilla.org/mozilla-central/rev/4baddb2df1db

Comment 20

[Branislav Rankov [:rankov]]

Attachment: 07-build-issues.patch

Comment 21

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f981a6431a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6f981a6431a