Make wasm tests pass on mips32

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(14 attachments, 11 obsolete attachments)

2.29 KB, patch
lth
: review+
Details | Diff | Splinter Review
16.02 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.66 KB, patch
lth
: review+
Details | Diff | Splinter Review
4.52 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.10 KB, patch
lth
: review+
Details | Diff | Splinter Review
6.00 KB, patch
lth
: review+
Details | Diff | Splinter Review
7.91 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
5.90 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
6.99 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
8.37 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
1.52 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
988 bytes, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
1.26 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
15.21 KB, patch
dragan.mladjenovic
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Steps to reproduce:

Tried running jit_test.py  dist/bin/js wasm on mips32 simulator.


Actual results:

Various test failures.


Expected results:

All but tests passing.
(Assignee)

Comment 2

2 years ago
Attachment #8901111 - Flags: review?(lhansen)
(Assignee)

Comment 4

2 years ago
Attachment #8901113 - Flags: review?(lhansen)
(Assignee)

Comment 5

2 years ago
Attachment #8901114 - Flags: review?(lhansen)
(Assignee)

Comment 6

2 years ago
Attachment #8901115 - Flags: review?(lhansen)
(Assignee)

Comment 7

2 years ago
Attachment #8901116 - Flags: review?(lhansen)
(Assignee)

Comment 8

2 years ago
Attachment #8901117 - Flags: review?(lhansen)
(Assignee)

Comment 9

2 years ago
Attachment #8901118 - Flags: review?(lhansen)
Oh dear.
Comment on attachment 8901123 [details] [diff] [review]
0014-Bug-1393723-Handle-wasm-i64-sign-extend-on-mips-r-lt.patch

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

::: js/src/jit/mips32/CodeGenerator-mips32.cpp
@@ +702,5 @@
> +      case MSignExtendInt64::Half:
> +        masm.move16SignExtend(input.low, output.low);
> +        break;
> +      case MSignExtendInt64::Word:
> +        masm.move32(input.low, output.low);

Ah, good eye.  This move is missing in the ARM version, it probably passes tests there because of register assignments.
Attachment #8901123 - Flags: review?(lhansen) → review+
Attachment #8901122 - Flags: review?(lhansen) → review+
Attachment #8901121 - Flags: review?(lhansen) → review+
Comment on attachment 8901120 [details] [diff] [review]
0011-Bug-1393723-Fix-wasm-i64-shifts-rotates-on-mips32-r-.patch

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

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +1016,5 @@
>      if (count->isConstant()) {
>          int32_t c = int32_t(count->toConstant()->toInt64() & 0x3F);
> +        if (!c){
> +#ifdef JS_CODEGEN_MIPS32
> +        masm.move64(input, output);

Indentation +4.  And technically a space before the '{' above.

::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
@@ +114,2 @@
>  #endif
> +    ins->setInt64Operand(0, useInt64RegisterAtStart(lhs));

This line shall be indented, and there shall be braces around it.  (and on the line before the #endif the else shall be on the same line as the }.)  Yes, this creates a problem for the #ifdef.  I think most of us would prefer to duplicate this last line:

#ifdef ...
  if (..) {
  } else {
    ins->setInt64Operand(0, ...)
  }
#else
  ins->setInt64Operand(0, ...)
#endif

@@ +127,1 @@
>      defineInt64ReuseInput(ins, mir, 0);

Indentation, as above, and consider duplicating it to make the code clean.
Attachment #8901120 - Flags: review?(lhansen) → review+
Comment on attachment 8901114 [details] [diff] [review]
0005-Bug-1393723-Fix-handling-of-float32-WasmStackArgs-on.patch

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

I'm going to r+ this because it's a simple patch that makes sense, but the commit comment is empty and as a general rule that is not acceptable.  The commit comment should describe, briefly, what the patch is doing, and if more context is needed the comment can include it or reference it.

(I'm probably telling you something you already know, but for the record: For this and other patches that I r+, the formatting problems I point out must be fixed before landing.)

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +2307,1 @@
>          }

The indentation is completely off her.  Most people would prefer to do 'else if' rather than 'else { if' as you have here.
Attachment #8901114 - Flags: review?(lhansen) → review+
Comment on attachment 8901115 [details] [diff] [review]
0006-Bug-1393723-Implement-missing-parts-of-bug-1338217-o.patch

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

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +2087,5 @@
>      }
>  
>      Label done, outOfRange;
> +    masm.wasmBoundsCheck(Assembler::AboveOrEqual, ptrReg, ToRegister(boundsCheckLimit),
> +                             &outOfRange);

Nit: indentation.

@@ +2174,5 @@
>      }
>  
>      Label outOfRange;
> +    masm.wasmBoundsCheck(Assembler::AboveOrEqual, ptrReg, ToRegister(boundsCheckLimit),
> +                             &outOfRange);

Nit: indentation.
Attachment #8901115 - Flags: review?(lhansen) → review+
Comment on attachment 8901118 [details] [diff] [review]
0009-Bug-1393723-Add-definition-of-Assembler-FramePointer.patch

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

I'll just have to assume that you know what you're doing when you're not saving the FramePointer on MIPS64.
Attachment #8901118 - Flags: review?(lhansen) → review+
Comment on attachment 8901119 [details] [diff] [review]
0010-Bug-1393723-Fix-wasm-prologue-offsets-for-mips-r-lth.patch

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

(Blind r+)
Attachment #8901119 - Flags: review?(lhansen) → review+
Comment on attachment 8901111 [details] [diff] [review]
0002-Bug-1393723-Fix-usage-of-branchTestString-in-CacheIR.patch

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

Jan, can you make sure this is OK?
Attachment #8901111 - Flags: review?(lhansen) → review?(jdemooij)
Comment on attachment 8901113 [details] [diff] [review]
0004-Bug-1393723-Implement-MacroAssembler-PopStackPtr-on-.patch

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1550,5 @@
>  
>  void
>  MacroAssembler::PopStackPtr()
>  {
> +    asMasm().ma_load(StackPointer, Address(StackPointer, 0),SizeWord);

Nit: space after the last ",".

@@ +1551,5 @@
>  void
>  MacroAssembler::PopStackPtr()
>  {
> +    asMasm().ma_load(StackPointer, Address(StackPointer, 0),SizeWord);
> +    framePushed_ -= sizeof(intptr_t);

Here and in other cases you'd probably be better off switching to using MacroAssembler::implicitPop, since it contains a bunch of error checks that are useful.  But you could file that as a followup bug.
Attachment #8901113 - Flags: review?(lhansen) → review+
Comment on attachment 8901117 [details] [diff] [review]
0008-Bug-1393723-Fix-handling-of-wasm-div-mod-mul64-on-mi.patch

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

::: js/src/jit/mips32/MacroAssembler-mips32-inl.h
@@ +191,5 @@
>  
>  void
>  MacroAssembler::sub64(Register64 src, Register64 dest)
>  {
> +    MOZ_ASSERT(dest.high != src.high);

And also dest.high != src.low, I think.  dest.high is clobbered on the second line but src.low is read on the third.
Attachment #8901117 - Flags: review?(lhansen) → review+
Comment on attachment 8901111 [details] [diff] [review]
0002-Bug-1393723-Fix-usage-of-branchTestString-in-CacheIR.patch

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

Good find, thanks.
Attachment #8901111 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 26

2 years ago
(In reply to Lars T Hansen [:lth] from comment #20)
> Comment on attachment 8901118 [details] [diff] [review]
> 0009-Bug-1393723-Add-definition-of-Assembler-FramePointer.patch
> 
> Review of attachment 8901118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll just have to assume that you know what you're doing when you're not
> saving the FramePointer on MIPS64.

Hi,

Thanks for the review. I fairly certain that the same change will be needed for mips64 too, but I tough to leave it to the follow-up patches targeting only or mainly mips64. Should I include that info into commit message or maybe add a TODO onto Architecture-mips-shared.h?
(In reply to Dragan Mladjenovic from comment #26)
> (In reply to Lars T Hansen [:lth] from comment #20)
> > Comment on attachment 8901118 [details] [diff] [review]
> > 0009-Bug-1393723-Add-definition-of-Assembler-FramePointer.patch
> > 
> > Review of attachment 8901118 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'll just have to assume that you know what you're doing when you're not
> > saving the FramePointer on MIPS64.
> 
> Hi,
> 
> Thanks for the review. I fairly certain that the same change will be needed
> for mips64 too, but I tough to leave it to the follow-up patches targeting
> only or mainly mips64. Should I include that info into commit message or
> maybe add a TODO onto Architecture-mips-shared.h?

Do what you think it best, I just felt like I needed to say something.  It sounds like you know what you're doing :)
(Assignee)

Comment 28

2 years ago
Attachment #8901120 - Attachment is obsolete: true
Attachment #8902142 - Flags: review+
(Assignee)

Comment 29

2 years ago
Attachment #8901118 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8902143 - Flags: review+
(Assignee)

Comment 30

2 years ago
Attachment #8901117 - Attachment is obsolete: true
Attachment #8902144 - Flags: review+
Comment on attachment 8901112 [details] [diff] [review]
0003-Bug-1393723-Fix-handling-of-wasm-truncate-to-uint32-.patch

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

OK.  I have not studied the mips code in detail here but it looks plausible.
Attachment #8901112 - Flags: review?(lhansen) → review+
Attachment #8901110 - Flags: review?(lhansen) → review+
Comment on attachment 8901116 [details] [diff] [review]
0007-Bug-1393723-Fix-handling-of-wasm-float32-64-u-int64-.patch

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

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +683,5 @@
>  
>          // Check that the result is in the uint64_t range.
>          masm.moveFromDouble(ScratchDoubleReg, output);
>          masm.as_cfc1(ScratchRegister, Assembler::FCSR);
> +        masm.as_ext(ScratchRegister, ScratchRegister, 6, 1);

It wouldn't hurt to have a comment or symbolic constant here to explain '6' and '1'.  Same below.
Attachment #8901116 - Flags: review?(lhansen) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Assignee: nobody → dragan.mladjenovic

Comment 41

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e37c3786444a
Fix GenerateInterruptExit for mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7f4271c027
Fix usage of branchTestString in CacheIRCompiler::emitArrayJoinResult. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/68604930b47b
Fix handling of wasm truncate-to-uint32 on mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd740b71f29
Implement MacroAssembler::PopStackPtr on mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/290b34ee2388
Fix handling of float32 WasmStackArgs on mips32. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/04a30a0ba074
Implement missing parts of bug 1338217 on mips32. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/330e2e4db3a7
Fix handling of wasm float32/64 <-> u/int64 on mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b71a2824c224
Fix handling of wasm div/mod/mul64 on mips32. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb95175a737
Add definition of Assembler::FramePointer for mips32. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6079341dccb2
Fix wasm prologue offsets for mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ce9e7b7648
Fix wasm i64 shifts/rotates on mips32. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/540cd433f474
Fix incorrect lowering of wasm i64 load/stores on mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/166c94ca628c
Fix incorrect copySign lowering on mips. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/81cdf89896a3
Handle wasm i64 sign extend on mips. r=lth
Keywords: checkin-needed
Blocks: 1395390
You need to log in before you can comment on or make changes to this bug.