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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

We should calculate address by 64-bit instructions on mips64.

Comment 1

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS64-Fix-profiler-test-bug1026485.js-fai.patch

Comment 2

[Tooru Fujisawa [:arai]]

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.

Comment 3

[hev [:hev]]

(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.

Comment 4

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS64-Fix-profiler-test-bug1026485.js-fai.patch

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d655f2ae624

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/8d655f2ae624