1284414 - (wasm-mips) Wasm baseline: MIPS support

Status: RESOLVED FIXED

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

Description

[hev [:hev]]

Almost none of the platform hooks in the initial wasm baseline compiler (bug 1232205) have working MIPS code.  This bug is the placeholder for bringing the wasm baseline to parity with ARM.

Any MIPS changes should wait unitl the ARM lands, because https://bugzilla.mozilla.org/show_bug.cgi?id=1277011#c3

Comment 2

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

Initial implementation of MIPS32/64 Wasm Baseline support. MIPS32 is fully complete, but MIPS64 is expected to be merged with changes from Bug 1421531. Please take a look.

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 8936787 [details] [diff] [review]
wasm_baseline_jit_mips.diff

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

Didn't look carefully at the MIPS code of course.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +130,5 @@
> +# include "jit/mips-shared/Assembler-mips-shared.h"
> +# include "jit/mips32/Assembler-mips32.h"
> +#endif
> +
> +#if defined(JS_CODEGEN_MIPS64)

Blank line.

@@ +418,5 @@
> +{
> +    RegI64 abiReturnRegI64;
> +
> +    SpecificRegs()
> +      : abiReturnRegI64(ReturnReg64)

abiReturnRegI64 should not be needed on 64-bit platform?

@@ +3107,5 @@
>          explicit FunctionCall(uint32_t lineOrBytecode)
>            : lineOrBytecode(lineOrBytecode),
>              reloadMachineStateAfter(false),
>              usesSystemAbi(false),
> +#if defined(JS_CODEGEN_ARM)

Revert this.

@@ +3118,5 @@
>          uint32_t lineOrBytecode;
>          ABIArgGenerator abi;
>          bool reloadMachineStateAfter;
>          bool usesSystemAbi;
> +#if defined(JS_CODEGEN_ARM)

Revert this.

@@ +3231,5 @@
>            case ValType::I64: {
>              ABIArg argLoc = call.abi.next(MIRType::Int64);
>              if (argLoc.kind() == ABIArg::Stack) {
>                  ScratchI32 scratch(*this);
> +

Remove this blank line.

@@ +3257,5 @@
>                  break;
>                }
>  #if defined(JS_CODEGEN_REGISTER_PAIR)
>                case ABIArg::GPR_PAIR: {
> +#if defined(JS_CODEGEN_ARM)

Fix the indentation you broke here (and in the #elif cases below).

@@ +3384,5 @@
>      // The compiler depends on moveImm32() clearing the high bits of a 64-bit
>      // register on 64-bit systems.
>  
>      void moveImm32(int32_t v, RegI32 dest) {
> +        masm.mov(ImmWord(int32_t(v)), dest);

I believe this is wrong, see the comment above.  First of all the cast is redundant since v is already int32_t.  Second the comment requires the high bits to be cleared, which is why there was a cast to begin with.

@@ +3460,5 @@
>          // Jump indirect via table element.
>          masm.ma_ldr(DTRAddr(scratch, DtrRegImmShift(switchValue, LSL, 2)), pc, Offset,
>                      Assembler::Always);
> +#elif defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +

Blank line.

@@ +3465,5 @@
> +        ScratchI32 scratch(*this);
> +        CodeLabel tableCl;
> +
> +        masm.ma_li(scratch, tableCl.patchAt());
> +#ifdef JS_CODEGEN_MIPS32

Indent one space (also else, endif).

@@ +4297,5 @@
>      }
>  
>      template<typename T>
>      void
> +    atomicRMW32(T srcAddr, Scalar::Type viewType, AtomicOp op, RegI32 rv, RegI32 rd, RegI32 temps[])

I don't think I like the idea of an array here.  A const& to a structure that contains your temps, maybe; we could generalize that for all platforms.  AtomicRMWTemps, say.  Arrays are susceptible to OOB references and don't allow elements to be named reasonably.

@@ +4312,5 @@
>  #endif
>              switch (op) {
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +              case AtomicFetchAddOp: masm.atomicFetchAdd8ZeroExtend(rv, srcAddr, temps[0], temps[1], temps[2], temps[3], rd); break;
> +              case AtomicFetchSubOp: masm.atomicFetchSub8ZeroExtend(rv, srcAddr, temps[0], temps[1], temps[2], temps[3], rd); break;

I guess you can keep this code if you like, though the atomics APIs will change a lot once bug 1425149 lands, and there are a few changes in the baseline compiler to fit into those changes.  Either way with this many temp registers you'll end up with a mips-specific API.  We should discuss moving that API into MacroAssembler.h.

Comment 4

[yuyin]

Comment on attachment 8936787 [details] [diff] [review]
wasm_baseline_jit_mips.diff

@@ -1956,9 +1989,253 @@ MacroAssembler::wasmTruncateFloat32ToInt32(FloatRegister input, Register output,
 {
     as_truncws(ScratchFloat32Reg, input);
     as_cfc1(ScratchRegister, Assembler::FCSR);

this is not the thing about this patch. when use fcsr bit 6, software should clean it before as_truncws?

+void
+MacroAssemblerMIPSShared::wasmLoadImpl(const wasm::MemoryAccessDesc& access, Register memoryBase, Register ptr,
+                         Register ptrScratch, AnyRegister output, Register tmp)
+{
+    uint32_t offset = access.offset();
+    MOZ_ASSERT(offset < wasm::OffsetGuardLimit);
+    MOZ_ASSERT_IF(offset, ptrScratch != InvalidReg);
+
+    // Maybe add the offset.
+    if (offset) {
+        asMasm().ma_addu(ptrScratch, ptr, Imm32(offset));

ma_daddu on mips64 vs ma_addu on mips32.


+void
+MacroAssemblerMIPSShared::wasmStoreImpl(const wasm::MemoryAccessDesc& access, AnyRegister value,
+                          Register memoryBase, Register ptr, Register ptrScratch, Register tmp)
+{
+    uint32_t offset = access.offset();
+    MOZ_ASSERT(offset < wasm::OffsetGuardLimit);
+    MOZ_ASSERT_IF(offset, ptrScratch != InvalidReg);
+
+    // Maybe add the offset.
+    if (offset) {
+        asMasm().ma_addu(ptrScratch, ptr, Imm32(offset));

same above

+void
+MacroAssemblerMIPS64Compat::wasmLoadI64Impl(const wasm::MemoryAccessDesc& access, Register memoryBase, Register ptr,
+            Register ptrScratch, Register64 output, Register tmp)
+{
+
+    uint32_t offset = access.offset();
+    MOZ_ASSERT(offset < wasm::OffsetGuardLimit);
+    MOZ_ASSERT_IF(offset, ptrScratch != InvalidReg);
+
+    // Maybe add the offset.
+    if (offset) {
+        asMasm().ma_addu(ptrScratch, ptr, Imm32(offset));
ma_daddu

+void
+MacroAssemblerMIPS64Compat::wasmStoreI64Impl(const wasm::MemoryAccessDesc& access, Register64 value, Register memoryBase,
+    Register ptr, Register ptrScratch, Register tmp)
+{
+    uint32_t offset = access.offset();
+    MOZ_ASSERT(offset < wasm::OffsetGuardLimit);
+    MOZ_ASSERT_IF(offset, ptrScratch != InvalidReg);
+
+    // Maybe add the offset.
+    if (offset) {
+        asMasm().ma_addu(ptrScratch, ptr, Imm32(offset));
+        ptr = ptrScratch;
+    }

same


+#elif defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
+
+        ScratchI32 scratch(*this);
+        CodeLabel tableCl;
+
+        masm.ma_li(scratch, tableCl.patchAt());
+#ifdef JS_CODEGEN_MIPS32
+        masm.lshiftPtr(Imm32(4), switchValue);
+#else
+        masm.ma_mul(switchValue, switchValue, Imm32(6 * 4));
+#endif

on mips64, I think it is better to add some ’padding nop‘ and also use lshiftPtr, because mul is low performance？


     void moveImm32(int32_t v, RegI32 dest) {
-        masm.mov(ImmWord(uint32_t(v)), dest);
+        masm.mov(ImmWord(int32_t(v)), dest);
     }
 
I aslo notice this before, I forget what is the issue, I will check this next week in the office computer.

Comment 5

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

Comment 6

[Milan Knežević]

Thank you for your reply and sorry for my sloppy changes.

(In reply to Lars T Hansen [:lth] from comment #3)
> Comment on attachment 8936787 [details] [diff] [review]
> wasm_baseline_jit_mips.diff
> 
> Review of attachment 8936787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +3384,5 @@
> >      // The compiler depends on moveImm32() clearing the high bits of a 64-bit
> >      // register on 64-bit systems.
> >  
> >      void moveImm32(int32_t v, RegI32 dest) {
> > +        masm.mov(ImmWord(int32_t(v)), dest);
> 
> I believe this is wrong, see the comment above.  First of all the cast is
> redundant since v is already int32_t.  Second the comment requires the high
> bits to be cleared, which is why there was a cast to begin with.
> 

There is a problem with implementation of this function on MIPS64. Currently, v value is zero extended, but MIPS64 requires it to be sign extended. I suspect that this is implementation for x64, so this code is replaced with call to move32 method which keeps the same behavior on x64 and implements correct one for MIPS64. 

> 
> @@ +4297,5 @@
> >      }
> >  
> >      template<typename T>
> >      void
> > +    atomicRMW32(T srcAddr, Scalar::Type viewType, AtomicOp op, RegI32 rv, RegI32 rd, RegI32 temps[])
> 
> I don't think I like the idea of an array here.  A const& to a structure
> that contains your temps, maybe; we could generalize that for all platforms.
> AtomicRMWTemps, say.  Arrays are susceptible to OOB references and don't
> allow elements to be named reasonably.
> 

This new patch contains implementation of Atomic32Temps class which replaces RegI32 array.

Comment 7

[Milan Knežević]

Thank you for your replay.

(In reply to yuyin from comment #4)
> Comment on attachment 8936787 [details] [diff] [review]
> wasm_baseline_jit_mips.diff
> 
> @@ -1956,9 +1989,253 @@
> MacroAssembler::wasmTruncateFloat32ToInt32(FloatRegister input, Register
> output,
>  {
>      as_truncws(ScratchFloat32Reg, input);
>      as_cfc1(ScratchRegister, Assembler::FCSR);
> 
> this is not the thing about this patch. when use fcsr bit 6, software should
> clean it before as_truncws?
> 

Yes, or we could access the 16th bit. That would also require changes to simulator which, currently, does not simulate status bits.

> +void
> +MacroAssemblerMIPSShared::wasmLoadImpl(const wasm::MemoryAccessDesc&
> access, Register memoryBase, Register ptr,
> +                         Register ptrScratch, AnyRegister output, Register
> tmp)
> +{
> +    uint32_t offset = access.offset();
> +    MOZ_ASSERT(offset < wasm::OffsetGuardLimit);
> +    MOZ_ASSERT_IF(offset, ptrScratch != InvalidReg);
> +
> +    // Maybe add the offset.
> +    if (offset) {
> +        asMasm().ma_addu(ptrScratch, ptr, Imm32(offset));
> 
> ma_daddu on mips64 vs ma_addu on mips32.
> 

Replaced with call to addPtr.

> 
> on mips64, I think it is better to add some ’padding nop‘ and also use
> lshiftPtr, because mul is low 
>

When these patches land we plan to replace current tableSwitch implementation with one pointer per entry same as on other architectures, so this is just easiest implementation which works for now.

Comment 8

[yuyin]

Comment on attachment 8938007 [details] [diff] [review]
wasm_baseline_jit_mips.diff

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +2143,5 @@
> +    if (IsUnaligned(access)) {
> +        MOZ_ASSERT(tmp != InvalidReg);
> +        if (isFloat) {
> +            if (byteSize == 4)
> +            asMasm().loadUnalignedFloat32(access, address, tmp, output.fpu());

indent

@@ +2145,5 @@
> +        if (isFloat) {
> +            if (byteSize == 4)
> +            asMasm().loadUnalignedFloat32(access, address, tmp, output.fpu());
> +            else
> +            asMasm().loadUnalignedDouble(access, address, tmp, output.fpu());

indent

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 8938007 [details] [diff] [review]
wasm_baseline_jit_mips.diff

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

Generally looks good, lots of coding style nits but otherwise this seems mostly sound.  You have a little work ahead of you to port this code to the new atomics APIs that just landed, but I think that work is stable now, as is the baseline compiler - I'll be working on other things for a couple of months.

::: js/src/jit/MacroAssembler.h
@@ +1456,5 @@
>  
>      // `ptr` and `value` will always be updated.
>      void wasmUnalignedStore(const wasm::MemoryAccessDesc& access, Register value, Register memoryBase,
> +                            Register ptr, Register ptrScratch, Register tmp)
> +        DEFINED_ON(arm, mips32, mips64);

We want to have a comment above saying that tmp must be defined on mips32/mips64 (whatever is appropriate) and invalid on ARM.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +402,5 @@
>        : abiReturnRegI64(ReturnReg64)
>      {}
>  };
> +
> +#elif defined(JS_CODEGEN_MIPS32)

Redundant blank line above the #elif.

@@ +414,5 @@
> +};
> +#elif defined(JS_CODEGEN_MIPS64)
> +struct SpecificRegs
> +{
> +    SpecificRegs(){}

The constructor is redundant, I think.

@@ +3266,5 @@
> +# elif defined(JS_CODEGEN_MIPS64)
> +                ScratchF64 scratch(*this);
> +                loadF64(arg, Register64(scratch));
> +                masm.mov(argLoc.gpr(), scratch);
> +                break;

I'm surprised that there's a MIPS64 case here.  JS_CODEGEN_REGISTER_PAIR is only defined in Architecture-mips32.h.

@@ -3326,5 @@
>      //
>      // Sundry low-level code generators.
>  
> -    // The compiler depends on moveImm32() clearing the high bits of a 64-bit
> -    // register on 64-bit systems.

Please do not remove this comment.

@@ +3459,5 @@
> +# ifdef JS_CODEGEN_MIPS32
> +        masm.lshiftPtr(Imm32(4), switchValue);
> +# else
> +        //masm.ma_mul(switchValue, switchValue, Imm32(6 * 4));
> +        masm.lshiftPtr(Imm32(5), switchValue);

The line that's commented out should be removed, it's not needed, the meaning of the lshift is clear.  (And if it's not then the comment should be in prose.)  Ideally the two cases could then be merged if the shift constant can be expressed simply as some function of sizeof(void*), say.

@@ +3689,1 @@
>  #else

Fix broken indendation.

@@ +3886,5 @@
>  #ifdef JS_PUNBOX64
>          masm.cmpPtrSet(Assembler::Equal, src.reg, ImmWord(0), dest);
>  #else
>          masm.or32(src.high, src.low);
> +        masm.cmp32Set(Assembler::Equal, src.low, Imm32(0), dest);

Nice.

@@ +4261,5 @@
> +#elif defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +            if (IsUnaligned(*access)) {
> +                switch (src.tag) {
> +                case AnyReg::I64:
> +                    masm.wasmUnalignedStoreI64(*access, src.i64(), HeapReg, ptr, ptr, temp);

This whole block is mis-indented.  Also, in a switch, the 'case' shall be indented by two spaces, and then the body of the case by another two (so the 'case's need fixing here).

@@ +4288,5 @@
>      }
>  
> +    template <size_t Temps>
> +    class Atomic32Temps {
> +      private:

"private:" is redundant.

@@ +4295,5 @@
> +      public:
> +        Atomic32Temps() : allocated(0) {}
> +        // allocate all temp registers if 'allocate' not specified
> +        void allocate(BaseCompiler* bc, size_t allocate=Temps) {
> +            for (size_t i = 0; i<allocate; ++i) {

No braces around single-line loop body.  Spaces around '<'.  Capitalize the comment and end it with '.'.  Blank line before the comment.

@@ +4301,5 @@
> +            }
> +            allocated = allocate;
> +        }
> +        void free(BaseCompiler* bc){
> +            for (size_t i = 0; i<allocated; ++i) {

As previous comment.

@@ +4306,5 @@
> +                bc->freeI32(temps_[i]);
> +            }
> +        }
> +        const RegI32& operator[](size_t index) const {
> +            MOZ_ASSERT(index<allocated);

Spaces around '<'.

@@ +4320,1 @@
>      template<typename T>

Add a blank line after '#endif'.

@@ +4320,3 @@
>      template<typename T>
>      void
> +    atomicRMW32(T srcAddr, Scalar::Type viewType, AtomicOp op, RegI32 rv, RegI32 rd, const AtomicRMW32Temps &temp)

Line shall be wrapped at 100 characters or less.

@@ +4332,5 @@
>  #endif
>              switch (op) {
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +              case AtomicFetchAddOp: masm.atomicFetchAdd8ZeroExtend(rv, srcAddr, temp[0], temp[1], temp[2], temp[3], rd); break;
> +              case AtomicFetchSubOp: masm.atomicFetchSub8ZeroExtend(rv, srcAddr, temp[0], temp[1], temp[2], temp[3], rd); break;

In this case you'll end up having to implement new porting interfaces in MacroAssembler.h, and then using those here.  I suggest you simply make interfaces there that are mips-only, and then you have an #ifdef mips case here that uses the correct API.  Ditto for the cases later in this file.  I'm happy to discuss concrete solutions in more detail.

@@ +4408,1 @@
>      template<typename T>

Blank line after '#endif'.

@@ +4408,3 @@
>      template<typename T>
>      void
> +    atomicCmpXchg32(T srcAddr, Scalar::Type viewType, RegI32 rexpect, RegI32 rnew, RegI32 rd, const AtomicCmpXchg32Temps &temp)

Line shall be wrapped at 100 characters or less.

@@ +4452,1 @@
>      template<typename T>

Blank line after '#endif'.

@@ +5040,2 @@
>              rv = (type == ValType::I64) ? bc->popI64ToI32() : bc->popI32();
> +            if (viewType != Scalar::Int32 && viewType != Scalar::Uint32) {

No braces around single-statement body.

Comment 10

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

Thank you for your earlier reviews and comments. The code is ported to the new atomics APIs and nits are cleaned.

Comment 11

[Lars T Hansen [:lth]]

This generally looks very good, it'll take me a couple more days to finish review.

Comment 12

[Lars T Hansen [:lth]]

Comment on attachment 8944391 [details] [diff] [review]
wasm_baseline_jit_mips.diff

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

r+, but before landing you must fix the problem with moveImm32(), commented on this at length below.  Just comment on this bug if you have questions or want to discuss something, I'll see it.

As usual I haven't tried to understand the MIPS assembly code in much detail, as my MIPS skills are poor.  But overall things look good I think.

You have *lots* of indentation problems, sometimes for whole blocks but mostly just lines that don't line up properly with the lines around them.  I don't care much about what you land in the mips/ subdirectories but we try to stick to a common coding style to make things easier to read and merge, and it's nice if the MIPS code can follow that style everywhere.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +4998,5 @@
>  }
>  
>  void
>  MacroAssembler::wasmUnalignedStore(const wasm::MemoryAccessDesc& access, Register value,
> +                                   Register memoryBase, Register ptr, Register ptrScratch, Register tmp)

Nit: line is too long, wrap the new argument.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +1558,5 @@
> +    Instruction* inst = (Instruction*) call - 6 /* six nops */;
> +    Assembler::WriteLoad64Instructions(inst, ScratchRegister, (uint64_t) target);
> +    inst[4] = InstReg(op_special, ScratchRegister, zero, ra, ff_jalr);
> +
> +#else

Spurious blank line.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.h
@@ +222,5 @@
> +        wasm::BytecodeOffset trapOffset);
> +
> +  protected:
> +    void wasmLoadImpl(const wasm::MemoryAccessDesc& access, Register memoryBase, Register ptr,
> +                        Register ptrScratch, AnyRegister output, Register tmp);

Indentation, normally 'R' would line up with 'c' above.  Also in the prototype above, and in the prototype below.

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +2475,5 @@
> +    if (IsUnaligned(access)) {
> +        MOZ_ASSERT(tmp != InvalidReg);
> +        if (byteSize <= 4) {
> +            asMasm().ma_load_unaligned(access, output.low, address, tmp,
> +                                    static_cast<LoadStoreSize>(8 * byteSize),

INdentation here and next line, and below, several places.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +3367,5 @@
>      // Sundry low-level code generators.
>  
>      // The compiler depends on moveImm32() clearing the high bits of a 64-bit
> +    // register on 64-bit systems except MIPS64 where high bits are sign extended
> +    // from lower bits.

I don't think I will let you do this.

If you look in popMemoryAccess() there's a case there to handle a constant heap address, it uses moveImm32 to move the constant into a register.  It is possible for that constant address to exceed 2^31-1, and in that case, on a 64-bit system, we will end up here with an address in the register that is out of bounds.

At the moment in Firefox we allow heaps up to 2^31-1 only, so at the moment this has no practical consequence (except maybe for the error message, if that contains the failing address, or the debugger).  But we would very much like to lift that limit on the heap length, especially on 64-bit systems.  So this may start to matter.

If you've implemented sign extension here because move32(imm,Register) on MIPS64 does sign extension then it is somewhat likely that move32() should change too, but I don't know that for a fact.  We should chat about that.

If you've implemented sign extension here because there's no easy way of loading a 32-bit constant with zero extension into the register on MIPS, perhaps you could test the constant and use move32() if the constant is less than 2^31, and use some more complicated sequence if it is larger?

@@ +4245,5 @@
>              else
>                  masm.wasmStore(*access, src.any(), HeapReg, ptr, ptr);
>          }
> +#elif defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +            if (IsUnaligned(*access)) {

Indentation is wrong for this entire block.

@@ +4305,2 @@
>  #ifdef JS_CODEGEN_X86
> +           {

Indentation is wrong, remove one leading space.

@@ +4352,2 @@
>  #if defined(JS_CODEGEN_X86)
> +           {

Indentation again.

Comment 13

[Dragan Mladjenovic]

(In reply to Lars T Hansen [:lth] from comment #12)
> Comment on attachment 8944391 [details] [diff] [review]
> wasm_baseline_jit_mips.diff
> 
> Review of attachment 8944391 [details] [diff] [review]:
> -----------------------------------------------------------------

Hi, Thanks for the review.

I will try to explain reasoning behind changes to moveImm32.

> If you've implemented sign extension here because move32(imm,Register) on
> MIPS64 does sign extension then it is somewhat likely that move32() should
> change too, but I don't know that for a fact.  We should chat about that.
> 
> If you've implemented sign extension here because there's no easy way of
> loading a 32-bit constant with zero extension into the register on MIPS,
> perhaps you could test the constant and use move32() if the constant is less
> than 2^31, and use some more complicated sequence if it is larger?

The 32-bit integers on MIPS64 are are sign extended to full register width.
The 32-bit arithmetic instructions require them to be so otherwise their result
is undefined by the spec. This also makes them transparent to logic and 
comparison instructions (MIPS doesn't have different 64-bit and 32-bit version of it)
which allays operate on full register width.

> If you look in popMemoryAccess() there's a case there to handle a constant
> heap address, it uses moveImm32 to move the constant into a register.  It is
> possible for that constant address to exceed 2^31-1, and in that case, on a
> 64-bit system, we will end up here with an address in the register that is
> out of bounds. 

For time being MIPS64 doesn't have WASM_HUGE_MEMORY support. I'll try to bring it in
after the baseline stabilizes. Either the constant is small enough
to not require bounds checks or it will be bounds checked at run-time.
Both cases work when constant is sign extended int32. I admit that the first case
requires that the constant be less than INT32_MAX. Which is currently true.

With WASM_HUGE_MEMORY support the ptr value will have to be zero extend to full 
pointer width in mips64 specific code. One might optimize the case of ptr being 
constant and avoid zero extension for constant less than INT32_MAX, but that probably
won't be worth the churn it introduces.
 
Three is a bit of mess on our part by calling  the $base  of memory access a ptr 
and transferring in in index field of BaseIndex that helps this whole confusion.

Not sure but it seem that x64 requires that all i32 values be zero extended to full
pointer width for wasmLoad/Store paths not just in case of moveImm32, 
so there doesn't seem to be anything special to moveImm32 ?
Should we reword the comment to mention popMemoryAccess? It is a bit cryptic as it is now.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Dragan Mladjenovic from comment #13)
> (In reply to Lars T Hansen [:lth] from comment #12)
> > Comment on attachment 8944391 [details] [diff] [review]
> > wasm_baseline_jit_mips.diff
> > 
> > Review of attachment 8944391 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Hi, Thanks for the review.
> 
> I will try to explain reasoning behind changes to moveImm32.
> 
> > If you've implemented sign extension here because move32(imm,Register) on
> > MIPS64 does sign extension then it is somewhat likely that move32() should
> > change too, but I don't know that for a fact.  We should chat about that.
> > 
> > If you've implemented sign extension here because there's no easy way of
> > loading a 32-bit constant with zero extension into the register on MIPS,
> > perhaps you could test the constant and use move32() if the constant is less
> > than 2^31, and use some more complicated sequence if it is larger?
> 
> The 32-bit integers on MIPS64 are are sign extended to full register width.
> The 32-bit arithmetic instructions require them to be so otherwise their result
> is undefined by the spec. This also makes them transparent to logic and 
> comparison instructions (MIPS doesn't have different 64-bit and 32-bit version of it)
> which allays operate on full register width.

I see.

> > If you look in popMemoryAccess() there's a case there to handle a constant
> > heap address, it uses moveImm32 to move the constant into a register.  It is
> > possible for that constant address to exceed 2^31-1, and in that case, on a
> > 64-bit system, we will end up here with an address in the register that is
> > out of bounds. 
> 
> For time being MIPS64 doesn't have WASM_HUGE_MEMORY support. I'll try to bring it in
> after the baseline stabilizes. Either the constant is small enough
> to not require bounds checks or it will be bounds checked at run-time.
> Both cases work when constant is sign extended int32. I admit that the first case
> requires that the constant be less than INT32_MAX. Which is currently true.

Well, this should be independent of WASM_HUGE_MEMORY.  As you say, for the time being this works; if we should manage to make it possible for ArrayBuffer in SpiderMonkey to be longer than 2^31-1 on 64-bit systems it will no longer work, as 0x80000000 would be a valid index into such a memory but would become a negative number on MIPS when loaded into a register with moveImm32().

> With WASM_HUGE_MEMORY support the ptr value will have to be zero extend to full 
> pointer width in mips64 specific code. One might optimize the case of ptr being 
> constant and avoid zero extension for constant less than INT32_MAX, but that probably
> won't be worth the churn it introduces.

That seems OK, except that I think there are / may be in the future some common paths in the baseline compiler that use the ptr value before the platform layer gets to do anything about it, eg, to add large offsets into the ptr before bounds checking and so on.

> Three is a bit of mess on our part by calling  the $base  of memory access a ptr 
> and transferring in in index field of BaseIndex that helps this whole
> confusion.

I agree some of the naming is suboptimal in general... :-(  on all platforms, really.

> Not sure but it seem that x64 requires that all i32 values be zero extended to full
> pointer width for wasmLoad/Store paths not just in case of moveImm32, 
> so there doesn't seem to be anything special to moveImm32 ?

So, on x64 most 32-bit operations use MOVL etc and those operations clear the high bits properly.  moveImm32() just follows that logic.

> Should we reword the comment to mention popMemoryAccess? It is a bit cryptic
> as it is now.

I think that's a good idea.

OK, so in summary, loading a sign extended immediate value works currently on MIPS64 (we think) and if it should turn out not to work we can hack around it in moveImm32 for the time being, at low cost, with an #ifdef to clear the high bits, until we can figure out a more permanent solution or move the fix elsewhere.

With that, I think you can prepare a final patch - I know some of the porting interfaces have changed a little bit lately - and land, if you like; presumably atomics will still fail until the atomics patch lands, though.

Comment 15

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

Comment 16

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

Code is rebased after landing of Bug 1420838 and Bug 1435220 to inbound. Can somebody land this?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6546f0bc46f3
Wasm baseline MIPS32/64 r=lth

Comment 18

[Andreea Pavel [:apavel]]

Backed out 1 changesets (bug 1284414) for build bustages at /builds/worker/workspace/build/src/js/src/wasm/WasmBaselineCompile.cpp:4271

Push that caused the bustages: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6546f0bc46f369750d65d7ab47492903e9ba65ee

Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160890213&repo=mozilla-inbound&lineNumber=9166

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d4f0c67903f5dd69753a3ff8aa16254fa242aa

Comment 19

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

There was an error in x86 build in wasm baseline code due new way of usage of atomic temps. I hope that everything is alright now.

Comment 20

[Lars T Hansen [:lth]]

Hi Milan, this no longer applies cleanly to mozilla-inbound and needs to be rebased.  (If you get that done today I will be sure to try to land it today, I'm working late into the evening.  Sorry for missing the landing window on the current patch.)

Comment 21

[Dragan Mladjenovic]

Attachment: wasm_baseline_jit_mips2.diff

Hi,

I've rebased the patch on behalf of Milan. I cannot mark previous version as obsolete so be careful to commit this patch.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93cd1cffb54e
Wasm baseline jit MIPS32/64 r=lth

Comment 23

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 93cd1cffb54e (bug 1284414) for build bustage at src/js/src/wasm/WasmBaselineCompile.cpp on a CLOSED TREE 

Failure push: 
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=aad65073f7569d72c33eb0930a7a162b2d12d0a0&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=161716322&repo=mozilla-inbound&lineNumber=11454

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0756076b7b98667b5bd583213369e5be7328064

Comment 24

[Lars T Hansen [:lth]]

I think we need to see a successful try push here before we attempt to land again.  Do any of you have try privileges?

Comment 25

[Dragan Mladjenovic]

(In reply to Lars T Hansen [:lth] from comment #24)
> I think we need to see a successful try push here before we attempt to land
> again.  Do any of you have try privileges?

Well this is embarrassing. I found the culprit an tested it locally. I'll send the new version after Bug 1437780 lands.

Comment 26

[Lars T Hansen [:lth]]

OK :)  Thanks for being patient, landing big patches can be frustrating, and it doesn't make it any easier that you have to ask us for help every time either.

Comment 27

[Milan Knežević]

Attachment: wasm_baseline_jit_mips.diff

Comment 28

[Milan Knežević]

The new version of patch has been submitted. Dragan's patch can be flagged as obsolete.

Comment 29

[Lars T Hansen [:lth]]

Comment on attachment 8950145 [details] [diff] [review]
wasm_baseline_jit_mips2.diff

If you want it landed you need to tag it as checkin-needed... if you can't do that you need to needinfo me so that I will know to do it (or somebody else, or ask on IRC, in #jsapi or #ionmonkey).

Comment 30

[Milan Knežević]

Sorry, I am not allowed to tag it as checkin-needed, can you do that, please?

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d34a39d393b
Wasm Baseline MIPS32/64. r=lth

Comment 32

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6d34a39d393b

Comment 33

[hev [:hev]]

Clear needinfo.