Closed Bug 1219125 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS64: Fix profiler/test-bug1026485.js failure in debug mode

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file, 1 obsolete file)

We should calculate address by 64-bit instructions on mips64.
Attachment #8679815 - Flags: review?(lhansen)
Attachment #8679815 - Flags: review?(arai.unmht)
Comment on attachment 8679815 [details] [diff] [review]
0001-IonMonkey-MIPS64-Fix-profiler-test-bug1026485.js-fai.patch

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

There seems some unnecessary changes (or am I missing something?)

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +432,5 @@
>  
>      MOZ_ASSERT(IsPowerOfTwo(JitStackValueAlignment));
> +    masm.addPtr(Imm32(JitStackValueAlignment - 1 /* for padding */ + 1 /* for |this| */), numArgsReg);
> +    masm.addPtr(t2, numArgsReg);
> +    masm.andPtr(Imm32(~(JitStackValueAlignment - 1)), numArgsReg);

the value of numArgsReg can be expressed in 32bit, and 32bit operation should be sufficient.
can you tell me the reason of this change?

@@ +459,5 @@
>      {
>          Label undefLoopTop;
>  
>          masm.bind(&undefLoopTop);
> +        masm.subPtr(Imm32(1), t1);

here too. sub32 seems to be sufficient.

@@ +480,5 @@
>      {
>          Label copyLoopTop;
>  
>          masm.bind(&copyLoopTop);
> +        masm.subPtr(Imm32(1), s3);

here too.

@@ +1199,3 @@
>          masm.loadPtr(Address(scratch2, RectifierFrameLayout::offsetOfDescriptor()), scratch3);
>          masm.ma_dsrl(scratch1, scratch3, Imm32(FRAMESIZE_SHIFT));
> +        masm.andPtr(Imm32((1 << FRAMETYPE_BITS) - 1), scratch3);

This can be and32.

@@ +1269,5 @@
>          masm.loadPtr(Address(scratch2, IonAccessorICFrameLayout::offsetOfDescriptor()), scratch3);
>  #ifdef DEBUG
>          // Assert previous frame is an IonJS frame.
>          masm.movePtr(scratch3, scratch1);
> +        masm.andPtr(Imm32((1 << FRAMETYPE_BITS) - 1), scratch1);

here too.
Attachment #8679815 - Flags: review?(arai.unmht) → feedback+
(In reply to Tooru Fujisawa [:arai] from comment #2)
> Comment on attachment 8679815 [details] [diff] [review]
> 0001-IonMonkey-MIPS64-Fix-profiler-test-bug1026485.js-fai.patch
> 
> Review of attachment 8679815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There seems some unnecessary changes (or am I missing something?)
> 
> ::: js/src/jit/mips64/Trampoline-mips64.cpp
> @@ +432,5 @@
> >  
> >      MOZ_ASSERT(IsPowerOfTwo(JitStackValueAlignment));
> > +    masm.addPtr(Imm32(JitStackValueAlignment - 1 /* for padding */ + 1 /* for |this| */), numArgsReg);
> > +    masm.addPtr(t2, numArgsReg);
> > +    masm.andPtr(Imm32(~(JitStackValueAlignment - 1)), numArgsReg);
> 
> the value of numArgsReg can be expressed in 32bit, and 32bit operation
> should be sufficient.
> can you tell me the reason of this change?
> 
> @@ +459,5 @@
> >      {
> >          Label undefLoopTop;
> >  
> >          masm.bind(&undefLoopTop);
> > +        masm.subPtr(Imm32(1), t1);
> 
> here too. sub32 seems to be sufficient.
> 
> @@ +480,5 @@
> >      {
> >          Label copyLoopTop;
> >  
> >          masm.bind(&copyLoopTop);
> > +        masm.subPtr(Imm32(1), s3);
> 
> here too.
> 
> @@ +1199,3 @@
> >          masm.loadPtr(Address(scratch2, RectifierFrameLayout::offsetOfDescriptor()), scratch3);
> >          masm.ma_dsrl(scratch1, scratch3, Imm32(FRAMESIZE_SHIFT));
> > +        masm.andPtr(Imm32((1 << FRAMETYPE_BITS) - 1), scratch3);
> 
> This can be and32.
> 
> @@ +1269,5 @@
> >          masm.loadPtr(Address(scratch2, IonAccessorICFrameLayout::offsetOfDescriptor()), scratch3);
> >  #ifdef DEBUG
> >          // Assert previous frame is an IonJS frame.
> >          masm.movePtr(scratch3, scratch1);
> > +        masm.andPtr(Imm32((1 << FRAMETYPE_BITS) - 1), scratch1);
> 
> here too.

Yes, I think these are harmless chanages. OK, I will revert it.
Attachment #8679815 - Attachment is obsolete: true
Attachment #8679815 - Flags: review?(lhansen)
Attachment #8679952 - Flags: review?(arai.unmht)
Attachment #8679952 - Flags: review?(arai.unmht) → review+
https://hg.mozilla.org/mozilla-central/rev/8d655f2ae624
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.