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

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rankov, Assigned: rankov)

Tracking

Trunk
mozilla34
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
MIPS build fails due to latest changes.

Some of these changes origin from bug 1017539.
(Assignee)

Updated

4 years ago
Assignee: nobody → branislav.rankov
(Assignee)

Comment 1

4 years ago
Created attachment 8435015 [details] [diff] [review]
01-emitLoadElementT.patch
(Assignee)

Comment 2

4 years ago
Created attachment 8435016 [details] [diff] [review]
02-Fix-StoreSlotT.patch
(Assignee)

Comment 3

4 years ago
Created attachment 8435017 [details] [diff] [review]
03-Missing.patch
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Attachment #8435015 - Flags: review?(jdemooij)
(Assignee)

Updated

4 years ago
Attachment #8435016 - Flags: review?(jdemooij)
(Assignee)

Updated

4 years ago
Attachment #8435017 - Flags: review?(jdemooij)

Updated

4 years ago
Attachment #8435015 - Flags: review?(jdemooij) → review+
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.
Attachment #8435016 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Attachment #8435017 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 8437634 [details] [diff] [review]
02-Fix-StoreSlotT.patch

Fixed the nits.
Carry review from previous patch.
Attachment #8435016 - Attachment is obsolete: true
Attachment #8437634 - Flags: review+
(Assignee)

Comment 6

4 years ago
Here is the try link for the patches:
https://tbpl.mozilla.org/?tree=Try&rev=2767a212b2be
(Assignee)

Comment 8

4 years ago
Created attachment 8449508 [details] [diff] [review]
04-missing-or32.patch
Attachment #8449508 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 9

4 years ago
Created attachment 8449513 [details] [diff] [review]
05-pjs-bailout.patch

This is follow-up on bug 1020359.
Attachment #8449513 - Flags: review?(nicolas.b.pierron)
Attachment #8449508 - Flags: review?(nicolas.b.pierron) → review+
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?
Attachment #8449513 - Flags: review?(nicolas.b.pierron) → review?(shu)

Comment 12

4 years ago
(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

4 years ago
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?
Attachment #8449513 - Flags: review?(shu) → review+
(Assignee)

Comment 14

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
Created attachment 8450944 [details] [diff] [review]
05-pjs-bailout.patch

Fixed nits. Carry review from previous patch.
Attachment #8449513 - Attachment is obsolete: true
Attachment #8450944 - Flags: review+
(Assignee)

Comment 16

4 years ago
Created attachment 8450946 [details] [diff] [review]
06-arm-typo.patch
Attachment #8450946 - Flags: review?(shu)

Comment 17

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

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

Thanks!
Attachment #8450946 - Flags: review?(shu) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 8465469 [details] [diff] [review]
07-build-issues.patch
Attachment #8465469 - Flags: review?(jdemooij)

Updated

4 years ago
Attachment #8465469 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

4 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a6f981a6431a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.