1424978 - Fix some function about mips jit.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[yuyin]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.4.7 (KHTML, like Gecko) Version/11.0.2 Safari/604.4.7

Steps to reproduce:

run jit-test and mach jstestbrowser


Actual results:

some tests failed


Expected results:

all pass

Comment 1

[yuyin]

Attachment: 0001-MIPS-Fix-some-function-implement.patch

Comment 2

[yuyin]

Attachment: 0002-MIPS64-Fix-tag-unbox-int32-value.patch

Comment 3

[yuyin]

Attachment: test-sort-basics-2.js

Comment 4

[yuyin]

0001 patch fix some jit-test tests, such as wasm/conversion.js
0002 patch fix test-sort-basics-2.js which is extract from ecma_6/TypedArray/sort_bacics.js

Comment 5

[yuyin]

Attachment: 0002-MIPS64-Fix-tag-unbox-int32-value.patch

Sorry I am missing clear bit32--bit47 in function boxValue, this change does not attach the result of this tests, but it should be fix also.

Comment 6

[Dragan Mladjenovic]

Comment on attachment 8936545 [details] [diff] [review]
0001-MIPS-Fix-some-function-implement.patch

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

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ -64,4 @@
>  static void
>  GenerateReturn(MacroAssembler& masm, int returnCode)
>  {
> -    MOZ_ASSERT(masm.framePushed() == sizeof(EnterJITRegs));

I believe that since Bug 1417595 this assert will no longer fail ?

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8936725 [details] [diff] [review]
0002-MIPS64-Fix-tag-unbox-int32-value.patch

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

Redirecting review, this is not really something I know much about.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8936725 [details] [diff] [review]
0002-MIPS64-Fix-tag-unbox-int32-value.patch

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

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +1487,5 @@
>  MacroAssemblerMIPS64Compat::unboxNonDouble(const ValueOperand& operand, Register dest)
>  {
> +    Label isInt32, done;
> +    Register tag = splitTagForTest(operand);
> +    asMasm().branchTestInt32(Assembler::Equal, tag, &isInt32);

I do not see anything similar in other implementations of ARM, x64:

https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm.cpp#3042
https://searchfox.org/mozilla-central/source/js/src/jit/x64/MacroAssembler-x64.h#842

Comment 9

[yuyin]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 8936725 [details] [diff] [review]
> 0002-MIPS64-Fix-tag-unbox-int32-value.patch
> 
> Review of attachment 8936725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/MacroAssembler-mips64.cpp
> @@ +1487,5 @@
> >  MacroAssemblerMIPS64Compat::unboxNonDouble(const ValueOperand& operand, Register dest)
> >  {
> > +    Label isInt32, done;
> > +    Register tag = splitTagForTest(operand);
> > +    asMasm().branchTestInt32(Assembler::Equal, tag, &isInt32);
> 
> I do not see anything similar in other implementations of ARM, x64:
> 
> https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-
> arm.cpp#3042
> https://searchfox.org/mozilla-central/source/js/src/jit/x64/MacroAssembler-
> x64.h#842


this is a 64-bit specific issue, 

see the comment :
https://searchfox.org/mozilla-central/source/js/src/jit/x64/MacroAssembler-x64.h#842

   // Unbox any non-double value into dest. Prefer unboxInt32 or unboxBoolean
    // instead if the source type is known.
    void unboxNonDouble(const ValueOperand& src, Register dest) {
---------------------------
we focus this on a box value about a negative 32-bit value， for example -1(int32).

we can see:
static constexpr Value 
fromInt32(int32_t i) {
     fromTagAndPayload(JSVAL_TAG_INT32, uint32_t(i));
}

so the tagged value of -1 should be 0xfff88000 ffffffff  // I remember fff88 is the int32 tag? 

the original code will unbox this to a value :  0x00000000 ffffffff, this is not -1 on mips64. so it must be wrong.


------------------------------

this bug is triggered when execute visitCompare, mips64 dose have a 32-bit compare instruction like cmpl on x64, it always use sltu/slt, which use both mips32/mips64, so the opreand must always a right 64-bit value on mips64.

So I think this is a bug but it has not been triggered yet on x64.

Comment 10

[yuyin]

see my last comment, do you have any suggestions?

Comment 11

[yuyin]

I just find this,
https://bugzilla.mozilla.org/show_bug.cgi?id=1432207#c0

looks like this bug fix the issue. but it did not change the code on mips platform. I will create a patch in that bug.

about this bug please checkin the 0001-**.patch which was already reviewed.

thank you.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da8287a6c2a
MIPS: Fix some function implement. r=lth

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7da8287a6c2a

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 15

[Steven DeTar [:sdetar]]

Nicolas, should we close this bug?

Comment 16

[Nicolas B. Pierron [:nbp]]

I suggest to close it and open a follow-up bug once the new patch comes up.