Closed Bug 1020359 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: rankov, Assigned: rankov)

Details

Attachments

(7 files, 2 obsolete files)

MIPS build fails due to latest changes.

Some of these changes origin from bug 1017539.
Assignee: nobody → branislav.rankov
Attached patch 02-Fix-StoreSlotT.patch (obsolete) — Splinter Review
Attached patch 03-Missing.patchSplinter Review
Status: NEW → ASSIGNED
Attachment #8435015 - Flags: review?(jdemooij)
Attachment #8435016 - Flags: review?(jdemooij)
Attachment #8435017 - Flags: review?(jdemooij)
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+
Attachment #8435017 - Flags: review?(jdemooij) → review+
Fixed the nits.
Carry review from previous patch.
Attachment #8435016 - Attachment is obsolete: true
Attachment #8437634 - Flags: review+
Here is the try link for the patches:
https://tbpl.mozilla.org/?tree=Try&rev=2767a212b2be
Attachment #8449508 - Flags: review?(nicolas.b.pierron)
Attached patch 05-pjs-bailout.patch (obsolete) — Splinter Review
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)
(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 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+
(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.
Fixed nits. Carry review from previous patch.
Attachment #8449513 - Attachment is obsolete: true
Attachment #8450944 - Flags: review+
Attachment #8450946 - Flags: review?(shu)
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+
Attachment #8465469 - Flags: review?(jdemooij)
Attachment #8465469 - Flags: review?(jdemooij) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a6f981a6431a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.