1393723 - Make wasm tests pass on mips32

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dragan Mladjenovic]

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.

Comment 1

[Dragan Mladjenovic]

Attachment: 0001-Bug-1393723-Fix-GenerateInterruptExit-for-mips-r-lth.patch

Comment 2

[Dragan Mladjenovic]

Attachment: 0002-Bug-1393723-Fix-usage-of-branchTestString-in-CacheIR.patch

Comment 3

[Dragan Mladjenovic]

Attachment: 0003-Bug-1393723-Fix-handling-of-wasm-truncate-to-uint32-.patch

Comment 4

[Dragan Mladjenovic]

Attachment: 0004-Bug-1393723-Implement-MacroAssembler-PopStackPtr-on-.patch

Comment 5

[Dragan Mladjenovic]

Attachment: 0005-Bug-1393723-Fix-handling-of-float32-WasmStackArgs-on.patch

Comment 6

[Dragan Mladjenovic]

Attachment: 0006-Bug-1393723-Implement-missing-parts-of-bug-1338217-o.patch

Comment 7

[Dragan Mladjenovic]

Attachment: 0007-Bug-1393723-Fix-handling-of-wasm-float32-64-u-int64-.patch

Comment 8

[Dragan Mladjenovic]

Attachment: 0008-Bug-1393723-Fix-handling-of-wasm-div-mod-mul64-on-mi.patch

Comment 9

[Dragan Mladjenovic]

Attachment: 0009-Bug-1393723-Add-definition-of-Assembler-FramePointer.patch

Comment 10

[Dragan Mladjenovic]

Attachment: 0010-Bug-1393723-Fix-wasm-prologue-offsets-for-mips-r-lth.patch

Comment 11

[Dragan Mladjenovic]

Attachment: 0011-Bug-1393723-Fix-wasm-i64-shifts-rotates-on-mips32-r-.patch

Comment 12

[Dragan Mladjenovic]

Attachment: 0012-Bug-1393723-Fix-incorrect-lowering-of-wasm-i64-load-.patch

Comment 13

[Dragan Mladjenovic]

Attachment: 0013-Bug-1393723-Fix-incorrect-copySign-lowering-on-mips-.patch

Comment 14

[Dragan Mladjenovic]

Attachment: 0014-Bug-1393723-Handle-wasm-i64-sign-extend-on-mips-r-lt.patch

Comment 15

[Lars T Hansen [:lth]]

Oh dear.

Comment 16

[Lars T Hansen [:lth]]

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.

Comment 17

[Lars T Hansen [:lth]]

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.

Comment 18

[Lars T Hansen [:lth]]

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.

Comment 19

[Lars T Hansen [:lth]]

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.

Comment 20

[Lars T Hansen [:lth]]

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.

Comment 21

[Lars T Hansen [:lth]]

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+)

Comment 22

[Lars T Hansen [:lth]]

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?

Comment 23

[Lars T Hansen [:lth]]

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.

Comment 24

[Lars T Hansen [:lth]]

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.

Comment 25

[Jan de Mooij [:jandem]]

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.

Comment 26

[Dragan Mladjenovic]

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

Comment 27

[Lars T Hansen [:lth]]

(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 :)

Comment 28

[Dragan Mladjenovic]

Attachment: 0011-Bug-1393723-Fix-wasm-i64-shifts-rotates-on-mips32-r-.patch

Comment 29

[Dragan Mladjenovic]

Attachment: 0009-Bug-1393723-Add-definition-of-Assembler-FramePointer.patch

Comment 30

[Dragan Mladjenovic]

Attachment: 0008-Bug-1393723-Fix-handling-of-wasm-div-mod-mul64-on-mi.patch

Comment 31

[Dragan Mladjenovic]

Attachment: 0011-Bug-1393723-Fix-wasm-i64-shifts-rotates-on-mips32-r-.patch

Comment 32

[Dragan Mladjenovic]

Attachment: 0009-Bug-1393723-Add-definition-of-Assembler-FramePointer.patch

Comment 33

[Dragan Mladjenovic]

Attachment: 0008-Bug-1393723-Fix-handling-of-wasm-div-mod-mul64-on-mi.patch

Comment 34

[Dragan Mladjenovic]

Attachment: 0006-Bug-1393723-Implement-missing-parts-of-bug-1338217-o.patch

Comment 35

[Dragan Mladjenovic]

Attachment: 0005-Bug-1393723-Fix-handling-of-float32-WasmStackArgs-on.patch

Comment 36

[Dragan Mladjenovic]

Attachment: 0004-Bug-1393723-Implement-MacroAssembler-PopStackPtr-on-.patch

Comment 37

[Dragan Mladjenovic]

Attachment: 0002-Bug-1393723-Fix-usage-of-branchTestString-in-CacheIR.patch

Comment 38

[Lars T Hansen [:lth]]

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.

Comment 39

[Lars T Hansen [:lth]]

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.

Comment 40

[Dragan Mladjenovic]

Attachment: 0007-Bug-1393723-Fix-handling-of-wasm-float32-64-u-int64-.patch

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/e37c3786444a
https://hg.mozilla.org/mozilla-central/rev/2a7f4271c027
https://hg.mozilla.org/mozilla-central/rev/68604930b47b
https://hg.mozilla.org/mozilla-central/rev/4fd740b71f29
https://hg.mozilla.org/mozilla-central/rev/290b34ee2388
https://hg.mozilla.org/mozilla-central/rev/04a30a0ba074
https://hg.mozilla.org/mozilla-central/rev/330e2e4db3a7
https://hg.mozilla.org/mozilla-central/rev/b71a2824c224
https://hg.mozilla.org/mozilla-central/rev/fdb95175a737
https://hg.mozilla.org/mozilla-central/rev/6079341dccb2
https://hg.mozilla.org/mozilla-central/rev/27ce9e7b7648
https://hg.mozilla.org/mozilla-central/rev/540cd433f474
https://hg.mozilla.org/mozilla-central/rev/166c94ca628c
https://hg.mozilla.org/mozilla-central/rev/81cdf89896a3