Closed Bug 969375 Opened 10 years ago Closed 10 years ago

MIPS port for SpiderMonkey JIT

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: rankov, Assigned: rankov)

References

Details

(Keywords: feature, Whiteboard: [js:p3])

Attachments

(12 files, 27 obsolete files)

10.04 KB, patch
nbp
: review+
nbp
: checkin+
Details | Diff | Splinter Review
90.13 KB, patch
rankov
: review+
nbp
: checkin+
Details | Diff | Splinter Review
145.43 KB, patch
rankov
: review+
nbp
: checkin+
Details | Diff | Splinter Review
27.77 KB, patch
rankov
: review+
nbp
: checkin+
Details | Diff | Splinter Review
1.04 KB, patch
nbp
: review+
nbp
: checkin+
Details | Diff | Splinter Review
13.54 KB, patch
rankov
: review+
nbp
: checkin+
Details | Diff | Splinter Review
24.13 KB, patch
froydnj
: review+
jandem
: review+
nbp
: checkin+
Details | Diff | Splinter Review
31.99 KB, patch
rankov
: review+
cbook
: checkin+
Details | Diff | Splinter Review
16.74 KB, patch
rankov
: review+
cbook
: checkin+
Details | Diff | Splinter Review
86.59 KB, patch
rankov
: review+
cbook
: checkin+
Details | Diff | Splinter Review
41.83 KB, patch
rankov
: review+
cbook
: checkin+
Details | Diff | Splinter Review
4.88 KB, patch
rankov
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152142

Steps to reproduce:

This bug should track the upstream process for MIPS port. Port contains Baseline Compiler, IonMonkey and Odinmonkey.

Current code for MIPS port is available at:
https://github.com/rankov-img/gecko-dev

Code will be offered in 5 phases:
- Skeleton code and Assembler
- Baseline Compiler code
- IonMonkey code
- OdinMonkey code
- MIPS simulator

Each phase will be separated in multiple patches. Each phase can be built and
tests can be run.
Hardware: x86_64 → Other
Attached patch Architecture-mips.patch (obsolete) — Splinter Review
Attachment #8372275 - Flags: review?(gal)
Attachment #8372275 - Flags: review?(brendan)
Assignee: nobody → branislav.rankov
Attached patch Architecture-mips.patch (obsolete) — Splinter Review
Attachment #8372275 - Attachment is obsolete: true
Attachment #8372275 - Flags: review?(gal)
Attachment #8372275 - Flags: review?(brendan)
Attachment #8372288 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8372288 [details] [diff] [review]
Architecture-mips.patch

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

"nit" are recommendation which are not blocking reviews, they need to be addressed before landing patches.
Are you implementing a MIPS32 assembly, or MIPS64 also have 32 bits registers?

This looks good so far, can you Address the questions and "nits", and submit a new patch for a quick review after?

::: js/src/jit/mips/Architecture-mips.cpp
@@ +25,5 @@
> +    static uint32_t flags = 0;
> +    if (isSet)
> +        return flags;
> +#if WTF_OS_LINUX
> +    FILE *fp = fopen("/proc/cpuinfo", "r");

FYI, this is ok for now but this would become a problem with B2G 1.5, as the child process will not have access to open.
We would have to fix this issue in ARM too.

::: js/src/jit/mips/Architecture-mips.h
@@ +20,5 @@
> +namespace js {
> +namespace jit {
> +
> +// Only Win64 requires shadow stack space.
> +static const uint32_t ShadowStackSpace = 0;

nit: I think comment above is nice, but it is misleading as we are not dealing with x86-64 here.

@@ +25,5 @@
> +
> +// These offsets are specific to nunboxing, and capture offsets into the
> +// components of a js::Value.
> +static const int32_t NUNBOX32_TYPE_OFFSET = 4;
> +static const int32_t NUNBOX32_PAYLOAD_OFFSET = 0;

nit: Also mention that MIPS registers are 32 bits registers.

@@ +186,5 @@
> +    static const uint32_t AllocatableMask = AllMask & ~NonAllocatableMask;
> +};
> +
> +// Smallest integer type that can hold a register bitmask.
> +typedef uint8_t PackedRegisterMask;

Ok, the code which is using this variable is buggy at the moment (see Bug 969436), but …
You should use uint32_t here, and not uint8_t.  By using uint8_t instead of uint32_t, you would be subject to issues when we attempt to recover frames on a call.

@@ +193,5 @@
> +{
> +  public:
> +    enum FPRegisterID {
> +        f0 = 0, // f0, f2 - Return values
> +        f2,

I am not sure to understand, are float register overlapping with doubles as on ARM?
And we can return 2 double?
Attachment #8372288 - Flags: review?(nicolas.b.pierron) → feedback+
Comment on attachment 8372288 [details] [diff] [review]
Architecture-mips.patch

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

Looks like nbp already volunteered to review this. Feel free to carry on nbp and pull me in if needed.

::: js/src/jit/mips/Architecture-mips.h
@@ +258,5 @@
> +    // f18 and f16 are MIPS scratch float registers.
> +    static const uint32_t NonAllocatableMask =
> +        (1 << f16) |
> +        (1 << f18) |
> +        (1 << invalid_freg);

Why is invalid_freg included here?
Attachment #8372288 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Are you implementing a MIPS32 assembly, or MIPS64 also have 32 bits
> registers?

We are implementing MIPS32 assembly.

> ::: js/src/jit/mips/Architecture-mips.cpp
> @@ +25,5 @@
> > +    static uint32_t flags = 0;
> > +    if (isSet)
> > +        return flags;
> > +#if WTF_OS_LINUX
> > +    FILE *fp = fopen("/proc/cpuinfo", "r");
>
> FYI, this is ok for now but this would become a problem with B2G 1.5, as the
> child process will not have access to open.
> We would have to fix this issue in ARM too.

I would leave it like this for now. We can fix it when ARM is fixed.

> @@ +193,5 @@
> > +{
> > +  public:
> > +    enum FPRegisterID {
> > +        f0 = 0, // f0, f2 - Return values
> > +        f2,
>
> I am not sure to understand, are float register overlapping with doubles as
> on ARM?
> And we can return 2 double?

There is register overlapping. Single float registers f0 and f1 are used as double register f0. We only use even registers because they can be used as both singe and double.
The register f2 is a return value for some exotic cases that do not happen in this port. It is used when returning complex numbers. It holds the imaginary part in that case.

I will fix the rest of issues, add comments and resubmit the patch.


(In reply to Andreas Gal :gal from comment #4)
>
> ::: js/src/jit/mips/Architecture-mips.h
> @@ +258,5 @@
> > +    // f18 and f16 are MIPS scratch float registers.
> > +    static const uint32_t NonAllocatableMask =
> > +        (1 << f16) |
> > +        (1 << f18) |
> > +        (1 << invalid_freg);
>
> Why is invalid_freg included here?

This is an error. It is not included in AllMask, so it shouldn't be here either.
Attachment #8372288 - Attachment is obsolete: true
Attachment #8372288 - Flags: review?(nicolas.b.pierron)
Attachment #8373222 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8373222 [details] [diff] [review]
Architecture-mips.patch

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

::: js/src/jit/mips/Architecture-mips.h
@@ +192,5 @@
> +
> +
> +// MIPS32 uses pairs of even and odd float registers as double precision
> +// registers. Example: f0 (double) is composed of f0 and f1 (single).
> +// This port only uses even registers to avoid allocation problems.

FYI, Marty (:mjrosenb) is dealing with a similar issue on ARM and is working on making us able to allocate f0 and f1 as float, while keeping a way to allocate both as a double.
So, I am ok to accept it for now.
Attachment #8373222 - Flags: review?(nicolas.b.pierron) → review+
Attached patch Assembler-mips-h.patch (obsolete) — Splinter Review
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch Assembler-mips-cpp.patch (obsolete) — Splinter Review
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ada8ee0e208
^ I added the bug number and the reviewer to your patch.

It does not matter much today, as MIPS is not checked yet, but you can also find your commit on our continuous integration infra:
  https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1&rev=6ada8ee0e208
Whiteboard: [leave-open]
http://hg.mozilla.org/integration/mozilla-inbound/rev/ba7c33c222c0 -
  Ryan VanderMeulen - Backed out changeset 6ada8ee0e208 (bug 969375) for check_spidermonkey_style.py failures.

The style check reported:

  js/src/jit/mips/Architecture-mips.cpp:14: error:
      "jit/mips/Assembler-mips.h" is included using the wrong path;
      did you forget a prefix, or is the file not yet committed?

As the Assembler file is not present yet, the style check eagerly failed on a non-used file :/
Comment on attachment 8373222 [details] [diff] [review]
Architecture-mips.patch

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

::: js/src/jit/mips/Architecture-mips.cpp
@@ +10,5 @@
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +
> +#include "jit/mips/Assembler-mips.h"

Is this header needed?
I do not see any code specific relying on this header, can I remove it and submit the patch again?
You can remove the include.
Attached patch Assembler-mips.patch (obsolete) — Splinter Review
I have created a new patch that contains both Assembler-mips.h and Assembler-mips.cpp. I have also run make check this time to check the style.
Attachment #8373316 - Attachment is obsolete: true
Attachment #8373326 - Attachment is obsolete: true
Attachment #8373470 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8373470 [details] [diff] [review]
Assembler-mips.patch

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

I will re-review tomorrow to check all memory marking/patching, in the mean time I will ask Nathan to check the MIPS encoding of instructions.
Also, I am impress by the simplicity of the encoding of instructions, nice work!

::: js/src/jit/mips/Assembler-mips.h
@@ +63,5 @@
> +static MOZ_CONSTEXPR_VAR Register CallTempReg1 = t1;
> +static MOZ_CONSTEXPR_VAR Register CallTempReg2 = t2;
> +static MOZ_CONSTEXPR_VAR Register CallTempReg3 = t3;
> +static MOZ_CONSTEXPR_VAR Register CallTempReg4 = t4;
> +static MOZ_CONSTEXPR_VAR Register CallTempReg5 = a2;

This would be better if you could avoid argument registersfor CallTempReg*.  The same applies to the ARM backend.

@@ +418,5 @@
> +    { }
> +};
> +
> +struct ImmType : public ImmTag
> +{

nit: ImmType and ImmTag are abstractions of the MacroAssembler.  Move these to the MacroAssembler.

@@ +425,5 @@
> +    { }
> +};
> +
> +static const ValueOperand JSReturnOperand = ValueOperand(JSReturnReg_Type, JSReturnReg_Data);
> +static const ValueOperand softfpReturnOperand = ValueOperand(v1, v0);

nit: same for these 2 ReturnOperand.

The idea is that the Assembler deals with the Assembly as it is in the documentation, and the MacroAssembler deals with the abstractions that we use in the engine.
Attachment #8373470 - Flags: review?(nfroyd)
Comment on attachment 8373470 [details] [diff] [review]
Assembler-mips.patch

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

All of this looks mostly right with regards to the instruction encoding bits, though I have a lot of little comments about things I noticed.  I didn't look at the bits linking the Assembler with the rest of the compiler, nor did I look too carefully at the Instruction subclasses.  I'm not a JS engine hacker, so Nicolas, please feel free to yell about me being overly pedantic and/or for overstepping my bounds of instruction encoding review. :)

I'm not giving this r+ because I don't understand exactly what you're doing with the floating-point registers.  Are you really trying to make this port go on MIPS chips without the full complement of 32 64-bit floating-point registers?  (I guess you are attempting to have it work on soft-float chips, judging by the earlier patch (!), so maybe you are really aiming for the full spectrum.)  Or could you just not make the o32-required even-register limitation work correctly with the full register set elsewhere (perhaps because of Ion limitations)?

Whichever way you answer, I'd like to see the attempts at odd-register support polished--or maybe see future patches so I can understand what you're trying to do there.

::: js/src/jit/mips/Assembler-mips.cpp
@@ +55,5 @@
> +uint32_t
> +js::jit::RT(FloatRegister r)
> +{
> +    JS_ASSERT(r.code() < FloatRegisters::Total);
> +    return (2 * r.code()) << RTShift;

Since you're only using the even registers, you should assert here that r.code() is indeed even, in addition to being in range.

@@ +76,5 @@
> +uint32_t
> +js::jit::RD(FloatRegister r)
> +{
> +    JS_ASSERT(r.code() < FloatRegisters::Total);
> +    return (2 * r.code()) << RDShift;

Likewise with the evenness assert here.

@@ +90,5 @@
> +
> +uint32_t
> +js::jit::SA(uint32_t value)
> +{
> +    JS_ASSERT(value <= 32);

<, not <=, since we only have a five-bit field.

@@ +97,5 @@
> +
> +uint32_t
> +js::jit::SA(FloatRegister r)
> +{
> +    JS_ASSERT(r.code() < FloatRegisters::Total);

Likewise with the evenness assert here.

@@ +142,5 @@
> +
> +bool
> +InstImm::isTHIS(const Instruction &i)
> +{
> +    return InstImm::isTHIS(i);

How does this return anything meaningful?

@@ +612,5 @@
> +    switch (c) {
> +      case Assembler::Equal:
> +      case Assembler::Zero:
> +      case Assembler::BelowOrEqual:
> +        return InstImm(op_beq, s, zero, BOffImm16(0)).encode();

Drop the .encode() here and subsequent cases?  No sense "encoding" only to reconstruct the InstImm.

@@ +729,5 @@
> +// Shift instructions
> +BufferOffset
> +Assembler::as_sll(Register rd, Register rt, uint16_t sa)
> +{
> +    JS_ASSERT(sa >= 0 && sa < 32);

GCC (and other compilers, I think) will complain here, because sa is of unsigned type and therefore always greater than zero; likewise for all checking of shift amounts below.

@@ +755,5 @@
> +
> +BufferOffset
> +Assembler::as_sra(Register rd, Register rt, uint16_t sa)
> +{
> +    return writeInst(InstReg(op_special, rs_zero, rt, rd, sa, ff_sra).encode());

You should JS_ASSERT the range of sa here for consistency.

@@ +767,5 @@
> +
> +BufferOffset
> +Assembler::as_rotr(Register rd, Register rt, uint16_t sa)
> +{
> +    return writeInst(InstReg(op_special, rs_one, rt, rd, sa, ff_srl).encode());

You should JS_ASSERT the range of sa here for consistency.

@@ +929,5 @@
> +BufferOffset
> +Assembler::as_ins(Register rt, Register rs, uint16_t pos, uint16_t size)
> +{
> +    Register rd;
> +    rd = Register::FromCode(pos + size - 1);

For sanity's sake, you should provide some asserts on valid values of pos and size.

@@ +937,5 @@
> +BufferOffset
> +Assembler::as_ext(Register rt, Register rs, uint16_t pos, uint16_t size)
> +{
> +    Register rd;
> +    rd = Register::FromCode(size - 1);

Likewise here.  (Register::FromCode might catch things, but it's good to be more explicit here.)

@@ +961,5 @@
> +Assembler::as_ls(FloatRegister fd, Register base, int32_t off, bool oddFReg)
> +{
> +    JS_ASSERT(Imm16::isInSignedRange(off));
> +    uint32_t fRegCode = oddFReg ? fd.code() * 2 + 1 : fd.code() * 2;
> +    return writeInst(InstImm(op_lwc1, base, fRegCode, Imm16(off)).encode());

I think I understand the desire to try to use all of the odd registers for singles, but what does this really buy you unless you extend odd register handling to all interesting FP register functions?  As things stand right now, you can only load and then move the bits to an integer register, which doesn't seem that useful.

I can't see the rest of the port, but going by the instruction definitions, you're already assuming that there are 32 floating-point registers (since the ${INST}.L.fmt/${INST}.fmt.L instructions are undefined if you're only working with 16 double floating-point registers).  (What CPUs are you trying to target here?)  So why not freely assume 32 floating-point registers everwhere and only enforce the even registers for doubles required by o32 at ABI boundaries?  (If I understand how things work in Ion/Odin, this can also enable you to use better calling conventions for intra-Odin calls, since you don't have to conform to the C o32 ABI for those.)

Alternatively, if that's not the case, you should just discard all the bits for the .L functions (both the Assembler::as_${INST} and the ff_*_l constants), and comment appropriately in the function-field enums, so nobody gets confused about what you're trying to do.

@@ +975,5 @@
> +
> +BufferOffset
> +Assembler::as_movs(FloatRegister fd, FloatRegister fs)
> +{
> +    return writeInst(InstReg(op_cop1, rs_s, zero, fs, fd, ff_mov_d).encode());

These ff_mov_d on .s format instructions should be cleaned up by renaming the function field enums appropriately, as described above.  Here and many instances below.

@@ +988,5 @@
> +BufferOffset
> +Assembler::as_mtc1(Register rt, FloatRegister fs, bool oddFReg)
> +{
> +    uint32_t fRegCode = oddFReg ? fs.code() * 2 + 1 : fs.code() * 2;
> +    return writeInst(InstReg(op_cop1, rs_mtc1, rt, fRegCode).encode());

I feel like you are going to want mthc1/mfhc1 as well at some point.

@@ +1123,5 @@
> +
> +BufferOffset
> +Assembler::as_subs(FloatRegister fd, FloatRegister fs, FloatRegister ft)
> +{
> +    return writeInst(InstReg(op_cop1, rs_s, ft, fs, fd, ff_sub_d).encode());

I wondered why you didn't have ff_sub_s...now I know.

@@ +1340,5 @@
> +    if (inst[0].encode() == inst_bgezal.encode()) {
> +        // Handle long call.
> +        addLongJump(BufferOffset(branch));
> +        inst[0] = InstImm(op_lui, zero, ScratchRegister, Imm16::upper(Imm32(target)));
> +        inst[1] = InstImm(op_ori, ScratchRegister, ScratchRegister, Imm16::lower(Imm32(target)));

You do this lui/ori dance often enough (4 times in this patch, probably more when the rest of the port comes in); you should probably have a separate function for it.

@@ +1419,5 @@
> +static int stopBKPT = -1;
> +void
> +Assembler::as_break(uint32_t code)
> +{
> +    JS_ASSERT(code >= 0 && code <= MAX_BREAK_CODE);

The compiler will complain about this >= 0 test.

::: js/src/jit/mips/Assembler-mips.h
@@ +196,5 @@
> +static const uint32_t StackAlignmentMask = StackAlignment - 1;
> +
> +static const int32_t MAX_16_BIT = 32767;
> +static const int32_t MIN_16_BIT = -32768;
> +static const int32_t MAX_16_BIT_U = 65536;

Just use the <stdint.h> INT16_MAX, etc. instead of inventing separate names for them.

@@ +271,5 @@
> +
> +    op_swc1     = 57 << OpcodeShift,
> +    op_sdc1     = 61 << OpcodeShift,
> +
> +    op_cop1x    = 19 << OpcodeShift

Please move this up with the op_cop1 opcode.

@@ +342,5 @@
> +    ff_tgeu      = 49,
> +    ff_tlt       = 50,
> +    ff_tltu      = 51,
> +    ff_teq       = 52,
> +    ff_tne       = 54,

All the trap function values are unused, please remove.

@@ +353,5 @@
> +    // special3 encoding of function field.
> +    ff_ext       = 0,
> +    ff_ins       = 4,
> +
> +    // cop1 encoding of function field when rs=S.

The only encodings that we really care about calling out are cvt.s.d and cvt.d.s.  All the other _d and _s encodings are identical.  I think it would be worthwhile to just have |ff_add|, |ff_sub|, |ff_round_l| etc. (perhaps with a |_fmt| suffix); ff_cvt_s_d and ff_cvt_d_s can then break from the mold.

This avoids numerous instances of things like:

InstReg(op_cop1, rs_s, zero, fs, fd, ff_trunc_w_d)

which makes one do a double-take (s fmt and _w_d function?).  Using unified names for the function fields will help with that.

@@ +355,5 @@
> +    ff_ins       = 4,
> +
> +    // cop1 encoding of function field when rs=S.
> +    ff_add_s     = 0,
> +    ff_round_l_s = 8,

See elsewhere about the .L format functions.

@@ +366,5 @@
> +    ff_floor_w_s = 15,
> +    ff_cvt_d_s   = 33,
> +    ff_cvt_w_s   = 36,
> +    ff_cvt_l_s   = 37,
> +    ff_cvt_ps_s  = 38,

The .ps values are not useful at this point, please remove.

@@ +407,5 @@
> +    // cop1 encoding of function field when rs=ps.
> +    // cop1x encoding of function field.
> +    ff_madd_d    = 33,
> +
> +    ff_nullff    = 0

madd_d and nullff are both unused, please remove.

@@ +436,5 @@
> +{
> +    uint32_t data;
> +
> +  public:
> +    uint32_t encode() {

You may want to add JS_ASSERTs for !isInvalid() here and in decode().

@@ +440,5 @@
> +    uint32_t encode() {
> +        return data;
> +    }
> +    int32_t decode() {
> +        return ((((int32_t)data) << 18) >> 16) + 4;

Signed left shifts across the sign bit invoke undefined behavior, unless you can guarantee that you don't change the sign bit.  I'm pretty sure you can't make that guarantee here.  To be pedantic, you want to do:

(int32_t(data << 18) >> 16) + 4;

Or you could store things in such a way that decode is free, but encode is a little more complicated.  I think that way would be slightly better, personally.

@@ +456,5 @@
> +        if ((offset - 4) > (MAX_16_BIT << 2))
> +            return false;
> +        return true;
> +    }
> +    static const int INVALID = 0x00020000;

Nit: make this uint32_t.  Then you don't need the cast below.

@@ +475,5 @@
> +{
> +    uint32_t data;
> +
> +  public:
> +    uint32_t encode() {

Ditto on adding JS_ASSERTs for !isInvalid() here and in decode().

@@ +479,5 @@
> +    uint32_t encode() {
> +        return data;
> +    }
> +    int32_t decode() {
> +        return (((int32_t)data << 8) >> 6) + 4;

Ditto here on the left-shifting through the sign bit.

@@ +495,5 @@
> +        if ((offset - 4) > 536870908)
> +            return false;
> +        return true;
> +    }
> +    static const int INVALID = 0x20000000;

Nit: uint32_t again.

@@ +542,5 @@
> +
> +class Operand
> +{
> +  public:
> +    enum Tag_ {

Nit: I think the usual convention is trailing underscores on data members.

@@ +549,5 @@
> +        MEM
> +    };
> +
> +  private:
> +    Tag_ Tag : 3;

You could call this field |tag| to avoid naming conflicts.

@@ +552,5 @@
> +  private:
> +    Tag_ Tag : 3;
> +    uint32_t reg : 5;
> +    int32_t offset;
> +    uint32_t data;

This field appears to be unused.

@@ +581,5 @@
> +    }
> +
> +    Register toReg() const {
> +        JS_ASSERT(Tag == REG);
> +        return Register::FromCode(reg);

No equivalent way to get a FloatRegister out of this?

@@ +590,5 @@
> +        *r = Register::FromCode(reg);
> +        *dest = Imm32(offset);
> +    }
> +    Address toAddress() const {
> +        return Address(Register::FromCode(reg), offset);

This should assert that Tag == MEM?

@@ +602,5 @@
> +        JS_ASSERT(Tag == MEM);
> +        return reg;
> +    }
> +    Register baseReg() const {
> +        return Register::FromCode(reg);

This should assert that Tag == MEM?

@@ +634,5 @@
> +        NonZero,
> +        Always,
> +    };
> +
> +    // Bit set when a DoubleCondition does not map to a single ARM condition.

Nit: MIPS.

@@ +884,5 @@
> +
> +    // FP instructions
> +
> +    // Use these two functions only when you are sure address is aligned.
> +    // Otherwise, use ma_ld and ma_sd.

I don't see ma_ld and ma_sd defined anywhere.  Oversight?  Or old comment?

@@ +1020,5 @@
> +    static void patchDataWithValueCheck(CodeLocationLabel label, ImmPtr newValue,
> +                                        ImmPtr expectedValue);
> +    static void patchWrite_Imm32(CodeLocationLabel label, Imm32 imm);
> +    static uint32_t alignDoubleArg(uint32_t offset) {
> +        return (offset + 1) &~ 1;

Nit: make this & ~1U;

@@ +1038,5 @@
> +        return m_buffer.bail();
> +    }
> +}; // Assembler
> +
> +// An Instruction is a structure for both encoding and decoding any and all ARM instructions.

Nit: MIPS instructions.

@@ +1073,5 @@
> +    bool is() const { return C::isTHIS(*this); }
> +
> +    // safely get a more specific variant of this pointer
> +    template <class C>
> +    C *as() const { return C::asTHIS(*this); }

You don't really use these, along with the isTHIS and asTHIS helpers; they should probably be deleted.

@@ +1109,5 @@
> +    uint32_t size() const { return 4; }
> +}; // Instruction
> +
> +// make sure that it is the right size
> +JS_STATIC_ASSERT(sizeof(Instruction) == 4);

Nit: please use static_assert here.

@@ +1210,5 @@
> +    { }
> +    InstImm(uint32_t raw)
> +      : Instruction(raw)
> +    {}
> +    // for float point

Nit: "For floating-point loads and stores."
Attachment #8373470 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 8373470 [details] [diff] [review]
> Assembler-mips.patch
> 
> Review of attachment 8373470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All of this looks mostly right with regards to the instruction encoding
> bits, though I have a lot of little comments about things I noticed.  I
> didn't look at the bits linking the Assembler with the rest of the compiler,
> nor did I look too carefully at the Instruction subclasses.  I'm not a JS
> engine hacker, so Nicolas, please feel free to yell about me being overly
> pedantic and/or for overstepping my bounds of instruction encoding review. :)
> 
> I'm not giving this r+ because I don't understand exactly what you're doing
> with the floating-point registers.  Are you really trying to make this port
> go on MIPS chips without the full complement of 32 64-bit floating-point
> registers?  (I guess you are attempting to have it work on soft-float chips,
> judging by the earlier patch (!), so maybe you are really aiming for the
> full spectrum.)  Or could you just not make the o32-required even-register
> limitation work correctly with the full register set elsewhere (perhaps
> because of Ion limitations)?
> 
> Whichever way you answer, I'd like to see the attempts at odd-register
> support polished--or maybe see future patches so I can understand what
> you're trying to do there.

IonMonkey currently has a limitation which is going to be removed soon.  The limitation is that we are unable to distinguish between Float registers and Double registers, as such the previous patch only encodes double set of registers.

I think, that a nit trick to avoid the " * 2" would be to make the enumeration contain all float registers and mark the odd registers as non-allocatable for the moment.

I also raised the hardfp issue on IRC, and this is not an issue because MIPS ABI does not use odd registers for ABI calls with floats.  It use the same registers as if the arguments were doubles.
(In reply to Nicolas B. Pierron [:nbp] from comment #17)
> IonMonkey currently has a limitation which is going to be removed soon.  The
> limitation is that we are unable to distinguish between Float registers and
> Double registers, as such the previous patch only encodes double set of
> registers.

OK, I can buy that.  There were just tricks to try to use the odd single registers as actual registers in the assembler for a very limited number of operations (and outside of ABI calls, of course).  Doing that seemed like asking for trouble later on.

> I think, that a nit trick to avoid the " * 2" would be to make the
> enumeration contain all float registers and mark the odd registers as
> non-allocatable for the moment.

That would work, yes.  I would still like to understand what the intentions behind the port are.
(In reply to Nathan Froyd (:froydnj) from comment #18)
> 
> OK, I can buy that.  There were just tricks to try to use the odd single
> registers as actual registers in the assembler for a very limited number of
> operations (and outside of ABI calls, of course).  Doing that seemed like
> asking for trouble later on.
> 

First, thanks for extensive review. I will answer the most pressing issues now. I will look at the rest tomorrow.

We are making a port for mips32r2 and plan to support mips32r1 soon. These CPUs have 32 32-bit floating point registers in which 64-bit data types are stored in even-odd pairs. This way, only even registers are 64-bit.

We encode only 64-bit registers for the reason Nicolas gave. 

There are instructions for accessing odd registers because they are needed for unaligned double loads and stores. Also there is a case in the ABI where double value needs to be stored in a2 and a3.

> > I think, that a nit trick to avoid the " * 2" would be to make the
> > enumeration contain all float registers and mark the odd registers as
> > non-allocatable for the moment.
> 

I tried this approach in the beginning. This required more changes in shared code because it assumes that float registers are 64-bit. I wanted to skip this at least for the first run. If you want, I can look into this again.

If you need to see the rest of the code, you can look at https://github.com/rankov-img/gecko-dev
(In reply to Branislav Rankov from comment #19)
> (In reply to Nathan Froyd (:froydnj) from comment #18)
> > 
> > OK, I can buy that.  There were just tricks to try to use the odd single
> > registers as actual registers in the assembler for a very limited number of
> > operations (and outside of ABI calls, of course).  Doing that seemed like
> > asking for trouble later on.
> > 
> 
> First, thanks for extensive review. I will answer the most pressing issues
> now. I will look at the rest tomorrow.

You're welcome!  Thanks for doing this work and pushing your work upstream.

> We are making a port for mips32r2 and plan to support mips32r1 soon. These
> CPUs have 32 32-bit floating point registers in which 64-bit data types are
> stored in even-odd pairs. This way, only even registers are 64-bit.

Ah, yes, if you're going to support older mips32r1 chips, then you do indeed need to do the 64-bit register dance here.

> There are instructions for accessing odd registers because they are needed
> for unaligned double loads and stores. Also there is a case in the ABI where
> double value needs to be stored in a2 and a3.

OK, that makes a great deal of sense.  Perhaps you should attempt to encapsulate the unaligned double load/store inside the assembler, along with a "load double to integer registers" (?) rather than exposing the individual odd/even register behavior to outside clients?  Doing that might also enable you to remove the InstReg/InstImm constructor variants that take bare uint32_t values for the registers.
I've fixed most of the issues found by Nathan and Nicolas. I need to run the tests and then I can upload the patch again. I will also wait for Nicolas to finish his review.

As agreed on the IRC, I will keep the registers as they are until Mark finishes his work on using all float32 registers.

I will also make methods that use odd registers protected so that only MacroAssembler and Assembler can use them.

More comments below:

(In reply to Nathan Froyd (:froydnj) from comment #16)
>
> ::: js/src/jit/mips/Assembler-mips.cpp
> @@ +55,5 @@
> > +uint32_t
> > +js::jit::RT(FloatRegister r)
> > +{
> > +    JS_ASSERT(r.code() < FloatRegisters::Total);
> > +    return (2 * r.code()) << RTShift;
>
> Since you're only using the even registers, you should assert here that
> r.code() is indeed even, in addition to being in range.

Only even registers are in FPRegisterID enum and the total regs number is 16, so no additional checks are needed.

> Alternatively, if that's not the case, you should just discard all the bits
> for the .L functions (both the Assembler::as_${INST} and the ff_*_l
> constants), and comment appropriately in the function-field enums, so nobody
> gets confused about what you're trying to do.
>

I've discarded all code that deals with .L instructions since this is mips32 port.

>
> I feel like you are going to want mthc1/mfhc1 as well at some point.
>

These will be needed when we extend the port to support full 64-bit FPU. We
can do that in the future.

>
> The only encodings that we really care about calling out are cvt.s.d and
> cvt.d.s.  All the other _d and _s encodings are identical.  I think it would
> be worthwhile to just have |ff_add|, |ff_sub|, |ff_round_l| etc. (perhaps
> with a |_fmt| suffix); ff_cvt_s_d and ff_cvt_d_s can then break from the
> mold.
>

I've cleaned all this up. Now we have constants like:
|ff_add_fmt|, |ff_sub_fmt|, |ff_round_w_fmt|, |ff_cvt_s_fmt| ...


> (int32_t(data << 18) >> 16) + 4;
>
> Or you could store things in such a way that decode is free, but encode is a
> little more complicated.  I think that way would be slightly better,
> personally.
>

I choose to hold encoded data for consistency with instructions. I've fixed the line the way you suggested.

>
> @@ +884,5 @@
> > +
> > +    // FP instructions
> > +
> > +    // Use these two functions only when you are sure address is aligned.
> > +    // Otherwise, use ma_ld and ma_sd.
>
> I don't see ma_ld and ma_sd defined anywhere.  Oversight?  Or old comment?

These are MacroAssembler instructions that handle unaligned load and store. They are implemented in MacroAssembler-mips.
Comment on attachment 8373470 [details] [diff] [review]
Assembler-mips.patch

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

::: js/src/jit/mips/Assembler-mips.cpp
@@ +338,5 @@
> +    for (size_t i = 0; i < codeLabels_.length(); i++) {
> +        CodeLabel label = codeLabels_[i];
> +        Bind(rawCode, label.dest(), rawCode + actualOffset(label.src()->offset()));
> +
> +        AutoFlushCache::updateTop(uintptr_t(rawCode + label.dest()->offset()), 8);

At this time, the code has never been executed yet, do we still need to flush the cache?
Is the instruction cache shared with the data cache, or there is no difference from MIPS CPUs?

@@ +514,5 @@
> +{
> +    BufferOffset ret;
> +    if (alignment == 8) {
> +        while (!m_buffer.isAligned(alignment)) {
> +            BufferOffset tmp = as_nop();

Assert that the buffer is currently aligned on the size of instructions (= 4 bytes), as otherwise this would cause an infinite loop.
Also, if the alignment is 8, then there is no need to do a while, as an if would be enough.

@@ +1298,5 @@
> +        do {
> +            Instruction *inst = editSrc(b);
> +
> +            // Second word holds a pointer to the next branch in label's chain.
> +            next = inst[1].encode();

Just to be sure I understand correctly, when generating an unbound branch, we reserve the space for 4 instructions, where we might write either a short-long jump?
Isn't there a more efficient way to handle Jumps?

We have a lot of short jumps which are local within a CodeGen's visit functions and which could fit on one instruction most of the time (less than ±32kB displacement).
Wouldn't it be better to always have short branches and have a similar thing as a constant pool when a label is going to overflow the 32Kb limit?  This way most of the codegen would be kept optimized while being able to patch large jumps?

@@ +1598,5 @@
> +    InstImm * inst = (InstImm *)inst_.raw();
> +
> +    // toggledJump is allways used for short jumps.
> +    JS_ASSERT(inst->extractOpcode() == ((uint32_t)op_beq >> OpcodeShift));
> +    // Replace "beq $zero, $zero, offset" with "andi $zero, $zero, offset"

That's handy!

@@ +1648,5 @@
> +        stop_ = newStop;
> +        return;
> +    }
> +
> +    if (newStop < start_ - 4096 || newStart > stop_ + 4096) {

I feel that one we reach this condition we will always reach it after, as we do not flush the range that we had before, but only the range of the new instructions.
Also, it seems that we have the same problem on ARM.
Attachment #8373470 - Flags: review?(nicolas.b.pierron) → feedback+
(In reply to Nicolas B. Pierron [:nbp] from comment #22)
> ::: js/src/jit/mips/Assembler-mips.cpp
> @@ +338,5 @@
> > +    for (size_t i = 0; i < codeLabels_.length(); i++) {
> > +        CodeLabel label = codeLabels_[i];
> > +        Bind(rawCode, label.dest(), rawCode + actualOffset(label.src()->offset()));
> > +
> > +        AutoFlushCache::updateTop(uintptr_t(rawCode + label.dest()->offset()), 8);
>
> At this time, the code has never been executed yet, do we still need to
> flush the cache?
> Is the instruction cache shared with the data cache, or there is no
> difference from MIPS CPUs?

MIPS has separate data and instruction cache. You might be right about this. This is copied from ARM. I will test if it makes any difference when removed.

> Just to be sure I understand correctly, when generating an unbound branch,
> we reserve the space for 4 instructions, where we might write either a
> short-long jump?
> Isn't there a more efficient way to handle Jumps?
>
> We have a lot of short jumps which are local within a CodeGen's visit
> functions and which could fit on one instruction most of the time (less than
> ±32kB displacement).
> Wouldn't it be better to always have short branches and have a similar thing
> as a constant pool when a label is going to overflow the 32Kb limit?  This
> way most of the codegen would be kept optimized while being able to patch
> large jumps?
>

There is a similar solution to what you are describing that can be implemented on MIPS. It has been done for V8. But this is more complicated and involves a lot of risks.
We decided to go with the current solution for simplicity at least for the start. We can modify this later on.

The current solution generates branches like this:
- Short branches: If a programmer knows that the branch will be short, he can set a flag for that. These branches always take up 2 instructions (branch and nop)
- Unconditional branches: These branches take 4 instructions and can be in two forms: short branch and long branch
- Conditional branches: These branches take 5 instructions and can be in two forms: short branch and long branch

We have made most of the branches generated by MIPS code to be generated short branches. We might be able to do this for the rest of generated code.

I have fixed the rest of issues and will upload the patch tomorrow if the tests pass.
Attached patch Assembler-mips.patch (obsolete) — Splinter Review
Attachment #8373470 - Attachment is obsolete: true
Attachment #8375494 - Flags: review?(nicolas.b.pierron)
Attachment #8375494 - Flags: review?(nfroyd)
Comment on attachment 8375494 [details] [diff] [review]
Assembler-mips.patch

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

r=me.  Just a few minor comments.

::: js/src/jit/mips/Assembler-mips.cpp
@@ +1415,5 @@
> +    // Always use long jump for two reasons:
> +    // - Jump has to be the same size because of patchWrite_NearCallSize.
> +    // - Return address has to be at the end of replaced block.
> +    // Short jump wouldn't be more efficient.
> +    writeLuiOriInstructions(inst, &inst[1], ScratchRegister, (uint32_t)dest);

Pointer-to-int conversions (especially on a 64-bit host) are going to warn on other compilers, if they're not already warning on yours.

::: js/src/jit/mips/Assembler-mips.h
@@ +55,5 @@
> +static MOZ_CONSTEXPR_VAR Register ra = { Registers::ra };
> +
> +static MOZ_CONSTEXPR_VAR Register ScratchRegister = at;
> +
> +// ARM uses arg reg from EnterJIT function as OsrFrameReg. We do the same.

Nit: MIPS uses
Attachment #8375494 - Flags: review?(nfroyd) → review+
Comment on attachment 8375494 [details] [diff] [review]
Assembler-mips.patch

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

::: js/src/jit/mips/Assembler-mips.cpp
@@ +59,5 @@
> +    return (2 * r.code()) << RTShift;
> +}
> +
> +uint32_t
> +js::jit::RT(uint32_t regCode)

see below.

@@ +80,5 @@
> +    return (2 * r.code()) << RDShift;
> +}
> +
> +uint32_t
> +js::jit::RD(uint32_t regCode)

Add a comment, similar to the one of the use cases:
  // Use to code odd float registers.
  // :TODO: Bug ABCDEF, It will be removed once we can use odd regs.

Also, in the JS engine, one way to deal with this future goal is to create a Bug and annotate the code with the bug number.

@@ +1325,5 @@
> +        // Handle long unconditional jump.
> +        addLongJump(BufferOffset(branch));
> +        writeLuiOriInstructions(inst, &inst[1], ScratchRegister, target);
> +        inst[2] = InstReg(op_special, ScratchRegister, zero, zero, ff_jr).encode();
> +        // There is 1 nop after this.

nit: There are 2 nops ?

@@ +1609,5 @@
> +}
> +
> +void Assembler::updateBoundsCheck(uint32_t heapSize, Instruction *inst)
> +{
> +    MOZ_ASSUME_UNREACHABLE("NYI");

Without this function, did you ensure that Firefox profile disable asm-js by default on MIPS?
This function is used when an asm-js pre-compiled modules is called and linked against.
Attachment #8375494 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #26)
> 
> @@ +1325,5 @@
> > +        // Handle long unconditional jump.
> > +        addLongJump(BufferOffset(branch));
> > +        writeLuiOriInstructions(inst, &inst[1], ScratchRegister, target);
> > +        inst[2] = InstReg(op_special, ScratchRegister, zero, zero, ff_jr).encode();
> > +        // There is 1 nop after this.
> 
> nit: There are 2 nops ?

For unconditional jumps, there are 4 instructions reserved in MacroAssembler. Here, we replace 3 of them. The fourth is left to be "nop" (MacroAssembler wrote the nop). So the jump sequence in this case is:
lui
ori
jr
nop

> 
> @@ +1609,5 @@
> > +}
> > +
> > +void Assembler::updateBoundsCheck(uint32_t heapSize, Instruction *inst)
> > +{
> > +    MOZ_ASSUME_UNREACHABLE("NYI");
> 
> Without this function, did you ensure that Firefox profile disable asm-js by
> default on MIPS?
> This function is used when an asm-js pre-compiled modules is called and
> linked against.

I'm trying to commit code in 4 phases as described in comment #0. The code needed for OdinMonkey will be added in phase 4. I can disable asm-js from phase 1 to phase 4.

I will update the comments and resubmit.
Carry reviews from the previous patch.
Attachment #8375494 - Attachment is obsolete: true
Attachment #8376255 - Flags: review+
Blocks: 972836
Comment on attachment 8376255 [details] [diff] [review]
Assembler-mips.patch

Backed out, and re-pushed, with the correct author this time (sorry for the first push)

https://hg.mozilla.org/integration/mozilla-inbound/rev/70307e75c08e
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f126126169
Could you plan to implement MIPS n32 support? Thanks!
We are currently focused on o32 ABI. Can you give me more info about what you need n32 ABI for?
Attached patch MacroAssempler-mips.patch (obsolete) — Splinter Review
Attachment #8377560 - Flags: review?(nicolas.b.pierron)
Attachment #8377560 - Flags: review?(nfroyd)
(In reply to Branislav Rankov from comment #34)
> We are currently focused on o32 ABI. Can you give me more info about what
> you need n32 ABI for?

I'm porting Arch Linux to MIPS (Loongson3), it's MIPS N32 ABI. currently, firefox 17 can works at methodjit enabled with some patches, I hope firefox 17 later works well on it. Thanks!
Comment on attachment 8377560 [details] [diff] [review]
MacroAssempler-mips.patch

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

Only f+ because the 64-bit issues need fixing and I'd like to see the results of the refactorings.  I didn't take a close look at some of the higher-level branching stuff.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +52,5 @@
> +    // calls with ScratchFloatReg as dest.
> +    MOZ_ASSERT(dest != SecondScratchFloatReg);
> +
> +    // Subtract LONG_MIN to get a positive number
> +    ma_subu(ScratchRegister, src, Imm32(LONG_MIN));

LONG_{MIN,MAX} aren't going to do the right thing on 64-bit hosts; you should be using the sized-MIN/MAX macros from stdint.h.  Here and many other places.

@@ +88,5 @@
> +    as_truncwd(ScratchFloatReg, src);
> +    as_mfc1(dest, ScratchFloatReg);
> +
> +    ma_b(dest, Imm32(LONG_MAX), fail, Assembler::Equal);
> +    ma_b(dest, Imm32(LONG_MIN), fail, Assembler::Equal);

If I'm reading the documentation for trunc.w.fmt correctly, it looks like the only invalid result you have to care about here is LONG_MAX.

@@ +159,5 @@
> +    as_truncws(ScratchFloatReg, src);
> +    as_mfc1(dest, ScratchFloatReg);
> +
> +    ma_b(dest, Imm32(LONG_MAX), fail, Assembler::Equal);
> +    ma_b(dest, Imm32(LONG_MIN), fail, Assembler::Equal);

Same comment applies from the double case.

@@ +253,5 @@
> +void
> +MacroAssemblerMIPS::ma_li(Register dest, Imm32 imm)
> +{
> +    if (Imm16::isInSignedRange(imm.value)) {
> +        as_addiu(dest, zero, imm.value);

May be worth testing for Imm16::isInUnsignedRange and using |ori dest, zero, imm| for that case.

Also may be worth testing for 16 low order bits of zeros and just using lui in that case.

@@ +256,5 @@
> +    if (Imm16::isInSignedRange(imm.value)) {
> +        as_addiu(dest, zero, imm.value);
> +    } else {
> +        as_lui(dest, Imm16::upper(imm).encode());
> +        as_ori(dest, dest, Imm16::lower(imm).encode());

Don't we have a function for lui/ori pairs now?

@@ +263,5 @@
> +
> +void
> +MacroAssemblerMIPS::ma_liPatchable(Register dest, Imm32 imm)
> +{
> +    m_buffer.ensureSpace(2 * sizeof(void *));

Almost everywhere you use sizeof() in this patch (.cpp and .h) is certainly wrong on 64-bit hosts.  You should have an addressSize constant or function (function would make a subsequent 64-bit port slightly easier) that you use in all the places that you are trying to use sizeof().  I'm not going to call them out individually, but there are a lot.

Since testing via the simulator is the likeliest way this port is going to get testing outside of your group, it's worthwhile to fix these now.

(I think you may have some instances of this in earlier patches; apologies for not being vigilant and catching them.)

@@ +265,5 @@
> +MacroAssemblerMIPS::ma_liPatchable(Register dest, Imm32 imm)
> +{
> +    m_buffer.ensureSpace(2 * sizeof(void *));
> +    as_lui(dest, Imm16::upper(imm).encode());
> +    as_ori(dest, dest, Imm16::lower(imm).encode());

Likewise for the lui/ori function.

@@ +362,5 @@
> +}
> +
> +void
> +MacroAssemblerMIPS::ma_and(Register rd, Imm32 imm)
> +{

The Register/Imm32 variant should just call the Register/Register/Imm32 variant.  Here and many other places below.

@@ +560,5 @@
> +{
> +    Label goodSubtraction;
> +    ma_subu(secondScratchReg_, rs, rt);
> +
> +    // Use second scratch. Instructions ma_b don't use it.

Nit: "The instructions generated by ma_b don't use the second scratch register."

@@ +561,5 @@
> +    Label goodSubtraction;
> +    ma_subu(secondScratchReg_, rs, rt);
> +
> +    // Use second scratch. Instructions ma_b don't use it.
> +    as_xor(ScratchRegister, rs, rt); // If same sign, no overflow

The comment about the second scratch register and the code here is confusing at best.  Maybe you should put the second scratch comment above with the ma_subu?

@@ +714,5 @@
> +
> +    switch (size) {
> +      case lsByte:
> +        if (lsZero == extension)
> +            as_lbu(dest, base, Imm16(offset).encode());

You should lift this out so the function looks like:

declare encodedOffset;
if (!Imm16::isInSignedRange(offset)) {
  ...
  encodedOffset = Imm16(0).encode();
} else {
  encodedOffset = Imm16(offset).encode();
}

and then use encodedOffset in all these cases.

@@ +749,5 @@
> +    uint32_t scale = Imm32::ShiftOf(src.scale).value;
> +
> +    // shift src.index to left for scale
> +    // and add base address value
> +    if (scale) {

I think it's worth having a computeScaledAddress for this |if (shift) { } else { }| pattern.  There are numerous places that you can use it (including those places where you already know scale is zero, which happens a couple of times).

I see that you have something like this in the header, computeEffectiveAddress.  Please make better use of it.

@@ +773,5 @@
> +    }
> +
> +    switch (size) {
> +      case lsByte:
> +        as_sb(data, base, Imm16(offset).encode());

Same comments from ma_load apply here.

@@ +805,5 @@
> +    // and add base address value
> +    if (scale)
> +        ma_sll(secondScratchReg_, dest.index, Imm32(scale));
> +    else
> +        ma_move(secondScratchReg_, dest.index);

This ma_move doesn't look necessary, and ought to be taken care of by the computeScaledAddress helper function.

@@ +830,5 @@
> +    // and add base address value
> +    if (scale)
> +        ma_sll(secondScratchReg_, dest.index, Imm32(scale));
> +    else
> +        ma_move(secondScratchReg_, dest.index);

Here too.

@@ +837,5 @@
> +    // add offset as well so that secondScratchReg_ contains absolute address
> +    if (dest.offset) {
> +        ma_li(ScratchRegister, Imm32(dest.offset));
> +        as_addu(secondScratchReg_, secondScratchReg_, ScratchRegister);
> +    }

You may want to add an offset as an optional argument to computeScaledAddress.

This ma_li/as_addu should just be ma_addu, so you can fold the immediate into the instruction if necessary.

@@ +848,5 @@
> +    ma_store(ScratchRegister, secondScratchReg_,
> +             0, size, extension);
> +}
> +
> +// Shortcut for when we know we're transferring 32 bits of data.

Perhaps a nice shortcut for the client of the macro assembler, but why not just delegate this to ma_load?

@@ +1076,5 @@
> +{
> +    switch (c) {
> +      case Above:
> +        // bgtu s,t,label =>
> +        //   sltu at,t,s

Nit: |at| should probably be |d| in all the comments in this function.

@@ +1243,5 @@
> +    ma_li(dest, Imm32(0));
> +    ma_li(ScratchRegister, Imm32(1));
> +    switch (c) {
> +      case DoubleOrdered:
> +        as_cund(lhs, rhs);

I almost wonder if you should have a function something like:

template<FloatFormat>
c.cond.fmt.function.type getFPComparison(DoubleCondition c, bool& swapOperands, bool& comparisonResult)

and then this function (and others) collapses to something like:

bool swapOperands, which;
c.cond.fmt.function.type compare = getFPComparison<Double>(c, swapOperands, which);
if (swapOperands)
  swap(lhs, rhs);
compare(lhs, rhs);
(which ? as_movt : as_movf)(dest, ScratchRegister);

which is a little more complex, but a little easier than reading through a sea of cases.  Not required, just food for though.

@@ +1272,5 @@
> +        as_movt(dest, ScratchRegister);
> +        break;
> +      case DoubleUnordered:
> +        as_cund(lhs, rhs);
> +        as_movf(dest, ScratchRegister);

It seems wrong that the code for the DoubleOrdered and the DoubleUnordered cases is identical.

@@ +1341,5 @@
> +        as_movt(dest, ScratchRegister);
> +        break;
> +      case DoubleUnordered:
> +        as_cuns(lhs, rhs);
> +        as_movf(dest, ScratchRegister);

The DoubleOrdered and the DoubleUnordered cases ought to be different, no?

@@ +1402,5 @@
> +    union SinglePun {
> +        uint32_t u;
> +        float s;
> +    } spun;
> +    spun.s = value;

Please use BitwiseCast from mozilla/Casting.h for this.

@@ +1421,5 @@
> +            uint32_t hi;
> +        } u;
> +        double d;
> +    } dpun;
> +    dpun.d = value;

Please use BitwiseCast for this too.

@@ +2511,5 @@
> +MacroAssemblerMIPSCompat::branchTestValue(Condition cond, const ValueOperand &value,
> +                                          const Value &v, Label *label)
> +{
> +    jsval_layout jv = JSVAL_TO_IMPL(v);
> +    if (v.isMarkable())

This block comes up a couple times; probably a good idea to have a separate function for it.

@@ +2512,5 @@
> +                                          const Value &v, Label *label)
> +{
> +    jsval_layout jv = JSVAL_TO_IMPL(v);
> +    if (v.isMarkable())
> +        ma_li(ScratchRegister, ImmGCPtr(reinterpret_cast<gc::Cell *>(v.toGCThing())));

I think this is going to do horrible things on a 64-bit host.  I've no idea what the correct solution is here.

@@ +2705,5 @@
> +                                            const FloatRegister &dest, int32_t shift)
> +{
> +    Label notInt32, end;
> +
> +    JS_STATIC_ASSERT(NUNBOX32_PAYLOAD_OFFSET == 0);

static_assert, please.

@@ +2770,5 @@
> +MacroAssemblerMIPSCompat::branchTestBooleanTruthy(bool b, const ValueOperand &operand,
> +                                                  Label *label)
> +{
> +    ma_and(ScratchRegister, operand.payloadReg(), operand.payloadReg());
> +    ma_b(ScratchRegister, ScratchRegister, label, b ? NonZero : Zero);

This is an oblique way to test if operand.payloadReg() is zero.  Why not just branch on operand.payloadReg() directly?

@@ +3151,5 @@
> +    ma_move(scratch, StackPointer);
> +
> +    // Force sp to be aligned
> +    ma_subu(StackPointer, StackPointer, Imm32(sizeof(intptr_t)));
> +    ma_and(StackPointer, StackPointer, Imm32(~(StackAlignment - 1)));

If you're going to use StackAlignment here, you might as well use it above in the ma_subu.

@@ +3350,5 @@
> +void
> +MacroAssemblerMIPSCompat::handleFailureWithHandler(void *handler)
> +{
> +    // Reserve space for exception information.
> +    int size = (sizeof(ResumeFromException) + 7) & ~7;

I think you mean +8 here, and this magic constant should be given a name somewhere.

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +23,5 @@
> +
> +
> +enum LoadStoreSize
> +{
> +    lsByte = 8,

"ls" seems like an awkward prefix; I'd say SizeByte would be better (and more consistent with enum naming style).

@@ +32,5 @@
> +
> +enum LoadStoreExtension
> +{
> +    lsZero = 0,
> +    lsSign = 1

Again, "ls" seems awkward here; ZeroExtend and SignExtend would be better.

@@ +54,5 @@
> +static const ValueOperand softfpReturnOperand = ValueOperand(v1, v0);
> +
> +static Register CallReg = t9;
> +static const int defaultShift = 3;
> +JS_STATIC_ASSERT(1 << defaultShift == sizeof(jsval));

static_assert, please.

@@ +189,5 @@
> +
> +    // memory
> +    // shortcut for when we know we're transferring 32 bits of data
> +
> +

Nix this extra whitespace, please.

@@ +444,5 @@
> +        as_jr(reg);
> +        as_nop();
> +    }
> +    void jump(const Address &address) {
> +        as_lw(ScratchRegister, address.base, address.offset);

Is this offset guaranteed to be in-range for the instruction?

@@ +1001,5 @@
> +            }
> +            bind(&negative);
> +            {
> +                ma_move(reg, zero);
> +                ma_b(&done, true);

Just fall through instead?

@@ +1119,5 @@
> +    }
> +
> +    void ma_storeImm(Imm32 imm, const Address &addr) {
> +        ma_li(secondScratchReg_, imm);
> +        ma_sw(secondScratchReg_, addr.base, addr.offset);

I believe all this is just ma_sw(imm, addr.base, addr.offset);
Attachment #8377560 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8377560 [details] [diff] [review]
MacroAssempler-mips.patch

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

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +58,5 @@
> +    // Convert value
> +    as_mtc1(ScratchRegister, dest);
> +    as_cvtdw(dest, dest);
> +
> +    // Add unsigned value of LONG_MIN

Instead of LONG_MIN and LONG_MAX, you should use INT32_MIN and INT32_MAX.  Or to be precise, we have never used LONG_* macro in other backends.

@@ +915,5 @@
> +}
> +
> +// Branches when done from within mips-specific code.
> +BufferOffset
> +MacroAssemblerMIPS::ma_b(Register lhs, Register rhs, Label *label, Condition c, bool shortJump)

A good practice is to avoid the boolean parameters, the reason being that when you read the call-site, you have no clue what the boolean means:
example:

  ma_b(value.payloadReg(), ScratchRegister, &done, NotEqual, true);

In such case, an enumertated value with 2 entries

enum JumpSize {
  ShortJump,
  LongJump
};

would be easier to read from the call-site:

  ma_b(value.payloadReg(), ScratchRegister, &done, NotEqual, ShortJump);

@@ +922,5 @@
> +      case Equal :
> +      case NotEqual:
> +        return branchWithCode(getBranchCode(lhs, rhs, c), label, shortJump);
> +      case Always:
> +        return ma_b(label);

nit: you forgot to transfert the shortJump argument.

@@ +942,5 @@
> +{
> +    MOZ_ASSERT(c != Overflow);
> +    if (imm.value == 0) {
> +        if (c == Always || c == AboveOrEqual)
> +            return ma_b(label);

nit: same here.

@@ +944,5 @@
> +    if (imm.value == 0) {
> +        if (c == Always || c == AboveOrEqual)
> +            return ma_b(label);
> +        else if (c == Below)
> +            return branchWithCode(getBranchCode(zero, NotEqual), label, shortJump);

I am not sure to understand where lhs is tested in this code-path?

@@ +988,5 @@
> +    InstImm inst_beq = InstImm(op_beq, zero, zero, BOffImm16(0));
> +
> +    if (m_buffer.oom()) {
> +        return BufferOffset();
> +    }

style-nit: in the JS engine, we remove the brackets when there is only one statement.

Is this oom check needed as it would be done by writeInst ?

@@ +993,5 @@
> +
> +    if (label->bound()) {
> +        int32_t offset = label->offset() - m_buffer.nextOffset().getOffset();
> +        if (shortJump) {
> +            code.setBOffImm16(BOffImm16(offset));

As the label is bound, assert that the offset is in the correct range.

@@ +999,5 @@
> +            as_nop();
> +            return bo;
> +        }
> +
> +        if (BOffImm16::isInRange(offset)) {

Use this condition to flip the shortJump flag and merge it with the above condition.

@@ +1243,5 @@
> +    ma_li(dest, Imm32(0));
> +    ma_li(ScratchRegister, Imm32(1));
> +    switch (c) {
> +      case DoubleOrdered:
> +        as_cund(lhs, rhs);

What Nathan is suggesting is what we did for visitCompareD [1] and with compareDouble [2].

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/shared/CodeGenerator-x86-shared.cpp#202
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/shared/MacroAssembler-x86-shared.h#37

@@ +1601,5 @@
> +        as_coles(lhs, rhs, fcc);
> +        return branchWithCode(getBranchCode(true, fcc), label, shortJump);
> +      case DoubleUnordered:
> +        as_cuns(lhs, rhs, fcc);
> +        return branchWithCode(getBranchCode(true, fcc), label, shortJump);

I think, typos would be harder if the code is refactored between ma_cmp_set_double, ma_cmp_set_float32, ma_bc1s and ma_bc1d.

@@ +2241,5 @@
> +
> +// higher level tag testing code
> +Operand ToPayload(Operand base)
> +{
> +    return Operand(Register::FromCode(base.base()), base.disp());

nit: use "base.disp() + PAYLOAD_OFFSET"

@@ +2245,5 @@
> +    return Operand(Register::FromCode(base.base()), base.disp());
> +}
> +Operand ToType(Operand base)
> +{
> +    return Operand(Register::FromCode(base.base()), base.disp() + sizeof(void *));

nit: use "base.disp() + TAG_OFFSET"

@@ +2712,5 @@
> +    ma_sll(secondScratchReg_, index, Imm32(shift));
> +    as_addu(secondScratchReg_, base, secondScratchReg_);
> +
> +    // Since we only have one scratch, we need to stomp over it with the tag.
> +    load32(Address(secondScratchReg_, NUNBOX32_TYPE_OFFSET), secondScratchReg_);

Why do you use the NUNBOX32_*_OFFSET variant here?  you might alias it locally as PAYLOAD_OFFSET and TAG_OFFSET.

@@ +2798,5 @@
> +}
> +
> +void
> +MacroAssemblerMIPSCompat::moveValue(const Value &val, Register type, Register data)
> +{

nit: Assert that type and data are different.

@@ +2957,5 @@
> +void
> +MacroAssemblerMIPSCompat::pushValue(const Address &addr)
> +{
> +    // Alocate stack slots for type and payload. One for each.
> +    ma_subu(StackPointer, StackPointer, Imm32(2 * sizeof(intptr_t)));

nit: Use sizeof(Value) instead.

@@ +2972,5 @@
> +    // Load payload and type.
> +    as_lw(val.payloadReg(), StackPointer, 0);
> +    as_lw(val.typeReg(), StackPointer, sizeof(intptr_t));
> +    // Free stack.
> +    as_addiu(StackPointer, StackPointer, 2 * sizeof(intptr_t));

nit: same here.
Attachment #8377560 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch MacroAssempler-mips.patch (obsolete) — Splinter Review
Attachment #8377560 - Attachment is obsolete: true
I have fixed most of the issues you found. Read my comments below on the rest of them. I refactored ma_b functions and floating point compare code.

(In reply to Nathan Froyd (:froydnj) from comment #37)
> @@ +256,5 @@
> > +    if (Imm16::isInSignedRange(imm.value)) {
> > +        as_addiu(dest, zero, imm.value);
> > +    } else {
> > +        as_lui(dest, Imm16::upper(imm).encode());
> > +        as_ori(dest, dest, Imm16::lower(imm).encode());

> Don't we have a function for lui/ori pairs now?

I don't want to use ma_liPatchable because we don't need ensureSpace here. 
Also writeLuiOriInstructions is a bit awkward to call here because i need the address of instruction for it.

> @@ +1001,5 @@
> > +            }
> > +            bind(&negative);
> > +            {
> > +                ma_move(reg, zero);
> > +                ma_b(&done, true);

> Just fall through instead?

I think this is ok. This code clamps negative numbers to 0.

> Almost everywhere you use sizeof() in this patch (.cpp and .h) is certainly
> wrong on 64-bit hosts.  You should have an addressSize constant or function
> (function would make a subsequent 64-bit port slightly easier) that you use
> in all the places that you are trying to use sizeof().  I'm not going to
> call them out individually, but there are a lot.

As discussed on IRC, we can leave some of these because we probably won't support X64 host. This said, I have went trough these and replaced the ones that have a better constant. I also added assert to the top of the file.


(In reply to Nicolas B. Pierron [:nbp] from comment #38)
> @@ +944,5 @@
> > +    if (imm.value == 0) {
> > +        if (c == Always || c == AboveOrEqual)
> > +            return ma_b(label);
> > +        else if (c == Below)
> > +            return branchWithCode(getBranchCode(zero, NotEqual), label, shortJump);

> I am not sure to understand where lhs is tested in this code-path?

This condition is always false. I changed the code so that nothing is emitted.

> I think, typos would be harder if the code is refactored between
> ma_cmp_set_double, ma_cmp_set_float32, ma_bc1s and ma_bc1d.

I refactored these functions. There is only one switch-case. I had to make changes in Assemler also.
If you think that all float-point functions in assembler should be refactored like this, I can make separate patch for this.
Comment on attachment 8379703 [details] [diff] [review]
MacroAssempler-mips.patch

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

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +1032,5 @@
> +            }
> +            bind(&negative);
> +            {
> +                ma_move(reg, zero);
> +                ma_b(&done, ShortJump);

Nicolass explained this on IRC. I will remove ma_b here as a nit or a separate patch.
Attachment #8379703 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8379703 [details] [diff] [review]
MacroAssempler-mips.patch

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

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +127,5 @@
> +                                          Label *fail, bool negativeZeroCheck)
> +{
> +    // convert the floating point value to an integer, if it did not fit,
> +    //     then when we convert it *back* to  a float, it will have a
> +    //     different value, which we can test.

style-nit: Any reason to indent the comment?

@@ +904,5 @@
> +
> +void
> +MacroAssemblerMIPS::ma_b(Label *label, JumpKind jumpKind)
> +{
> +    branchWithCode(getBranchCode(false), label, jumpKind);

This "false" is miss-leading.

@@ +910,5 @@
> +
> +void
> +MacroAssemblerMIPS::ma_bal(Label *label, JumpKind jumpKind)
> +{
> +    branchWithCode(getBranchCode(true), label, jumpKind);

and hard to understand as well as this "true".

You should prefer something like

  BranchIsJump
  BranchIsCall

@@ +1162,5 @@
> +
> +void
> +MacroAssemblerMIPS::compareFloatingPoint(FloatFormat fmt, FloatRegister lhs, FloatRegister rhs,
> +                                         DoubleCondition c, bool &branchCondition,
> +                                         FPConditionBit fcc)

When we have out-param, we prefer to write them as pointers instead of references, such as …

@@ +1235,5 @@
> +    ma_li(dest, Imm32(0));
> +    ma_li(ScratchRegister, Imm32(1));
> +
> +    bool moveCondition;
> +    compareFloatingPoint(DoubleFloat, lhs, rhs, c, moveCondition);

… when they are used, we are forced to add an ampersand before, to highlight that the value is mutated.

@@ +1329,5 @@
> +MacroAssemblerMIPS::ma_mv(FloatRegister src, Register dest1, Register dest2)
> +{
> +    if (dest2 == InvalidReg) {
> +        // For 32bit value.
> +        as_mfc1(dest1, src);

Do not use a default value to switch between float and double, do it with the prototype of the function.

@@ +1333,5 @@
> +        as_mfc1(dest1, src);
> +    } else {
> +        // For 64bit value.
> +        as_mfc1(dest1, src);
> +        as_mfc1_Odd(dest2, src);

when we have a couple of register for a double, I would prefer that we use a ValueOperand instead of 2 Register gave as argument.

@@ +1338,5 @@
> +    }
> +}
> +
> +void
> +MacroAssemblerMIPS::ma_mv(Register src1, Register src2, FloatRegister dest)

Same here,
  // Double
  mv_ma(ValueOperand src, FloatRegister dest);
  mv_ma(FloatRegister src, ValueOperand dest);

  // Float
  mv_ma(Register src, FloatRegister dest);
  mv_ma(FloatRegister src, Register dest);

@@ +1341,5 @@
> +void
> +MacroAssemblerMIPS::ma_mv(Register src1, Register src2, FloatRegister dest)
> +{
> +    as_mtc1(src1, dest);
> +    as_mtc1_Odd(src2, dest);

note: this code does not guard src2 to do a float32 move.

@@ +1345,5 @@
> +    as_mtc1_Odd(src2, dest);
> +}
> +
> +void
> +MacroAssemblerMIPS::ma_ls(FloatRegister ft, Register base, int32_t off)

Replace the couple "Register, int32_t" by an Address

@@ +1392,5 @@
> +}
> +
> +void
> +MacroAssemblerMIPS::ma_sd(FloatRegister ft, Register base, Register index, int32_t off,
> +                          int32_t shift)

Replace the tuplb (Register, Register, int32_t, int32_t) by BaseIndex.
Attachment #8379703 - Flags: review?(nfroyd)
Comment on attachment 8379703 [details] [diff] [review]
MacroAssempler-mips.patch

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

I like this.  Just a few small comments.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +1395,5 @@
> +MacroAssemblerMIPS::ma_sd(FloatRegister ft, Register base, Register index, int32_t off,
> +                          int32_t shift)
> +{
> +    computeScaledAddress(base, index, shift, secondScratchReg_);
> +    int32_t off2 = off + TAG_OFFSET;

All the following is just:

ma_sd(ft, secondScratchReg_, off);

@@ +1424,5 @@
> +MacroAssemblerMIPS::ma_ss(FloatRegister ft, Register base, Register index, int32_t off,
> +                          int32_t shift)
> +{
> +    computeScaledAddress(base, index, shift, secondScratchReg_);
> +    if (Imm16::isInSignedRange(off)) {

This if could just be ma_ss(ft, secondScratchReg_, off);

@@ +2406,5 @@
> +
> +void
> +MacroAssemblerMIPSCompat::boolValueToDouble(const ValueOperand &operand, const FloatRegister &dest)
> +{
> +    ma_cmp_set(ScratchRegister, operand.payloadReg(), zero, NotEqual);

Is there a reason this isn't just convertBoolToInt32?

@@ +2423,5 @@
> +                                             const FloatRegister &dest)
> +{
> +    // Note that C++ bool is only 1 byte, so zero extend it to clear the
> +    // higher-order bits.
> +    ma_and(ScratchRegister, operand.payloadReg(), Imm32(0xff));

This should just be convertBoolToInt32.  Then all the comments for this function become redundant.

@@ +2702,5 @@
> +
> +void
> +MacroAssemblerMIPSCompat::pushValue(ValueOperand val)
> +{
> +    // Alocate stack slots for type and payload. One for each.

Nit: "Allocate".

@@ +2706,5 @@
> +    // Alocate stack slots for type and payload. One for each.
> +    ma_subu(StackPointer, StackPointer, Imm32(sizeof(Value)));
> +    // Store type and payload.
> +    ma_sw(val.typeReg(), StackPointer, TAG_OFFSET);
> +    ma_sw(val.payloadReg(), StackPointer, PAYLOAD_OFFSET);

storeValue(val, Address(StackPointer, 0));

@@ +2712,5 @@
> +
> +void
> +MacroAssemblerMIPSCompat::pushValue(const Address &addr)
> +{
> +    // Alocate stack slots for type and payload. One for each.

Nit: "Allocate".
Attachment #8379703 - Flags: review+
Comment on attachment 8379703 [details] [diff] [review]
MacroAssempler-mips.patch

Argh, bugzilla fail.  r+'d this already.
Attachment #8379703 - Flags: review?(nfroyd)
Comment on attachment 8379703 [details] [diff] [review]
MacroAssempler-mips.patch

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

Fix the nits & previous recommendation, and r=me.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +269,5 @@
> +MacroAssemblerMIPS::ma_liPatchable(Register dest, Imm32 imm)
> +{
> +    m_buffer.ensureSpace(2 * sizeof(uint32_t));
> +    as_lui(dest, Imm16::upper(imm).encode());
> +    as_ori(dest, dest, Imm16::lower(imm).encode());

nit: Add a comment the code generated here is likely to be modified by updateLuiOriValue, either during compilation (Assembler::bind), or during the execution (jit::PatchJump).

@@ +682,5 @@
> +
> +// Memory.
> +
> +void
> +MacroAssemblerMIPS::ma_load(const Register &dest, Register base, int32_t offset,

Use Address instead of base + offset.

@@ +727,5 @@
> +}
> +
> +void
> +MacroAssemblerMIPS::ma_store(const Register &data, Register base,
> +                             int32_t offset, LoadStoreSize size,

same here.

@@ +781,5 @@
> +    ma_store(ScratchRegister, secondScratchReg_, 0, size, extension);
> +}
> +
> +void
> +MacroAssemblerMIPS::computeScaledAddress(Register base, Register index, int32_t shift,

Use BaseIndex.
Comment on attachment 8379703 [details] [diff] [review]
MacroAssempler-mips.patch

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

(see previous comment)
Attachment #8379703 - Flags: review?(nicolas.b.pierron) → review+
Carry reviews from the previous patch. 
All the issues found in the reviews have been fixed.
Attachment #8379703 - Attachment is obsolete: true
Attachment #8381439 - Flags: review+
Keywords: feature, leave-open
Whiteboard: [leave-open] → [js:p3]
Attachment #8381439 - Flags: checkin+
Comment on attachment 8381439 [details] [diff] [review]
MacroAssempler-mips.patch

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

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +407,5 @@
> +
> +void
> +MacroAssemblerMIPS::ma_or(Register rd, Register rs, Imm32 imm)
> +{
> +    if (Imm16::isInSignedRange(imm.value)) {

// Should be check unsignd range.

@@ +437,5 @@
> +
> +void
> +MacroAssemblerMIPS::ma_xor(Register rd, Register rs, Imm32 imm)
> +{
> +    if (Imm16::isInSignedRange(imm.value)) {

// Same as above
(In reply to Heiher from comment #50)
> 
> ::: js/src/jit/mips/MacroAssembler-mips.cpp
> @@ +407,5 @@
> > +
> > +void
> > +MacroAssemblerMIPS::ma_or(Register rd, Register rs, Imm32 imm)
> > +{
> > +    if (Imm16::isInSignedRange(imm.value)) {
> 
> // Should be check unsignd range.
> 
> @@ +437,5 @@
> > +
> > +void
> > +MacroAssemblerMIPS::ma_xor(Register rd, Register rs, Imm32 imm)
> > +{
> > +    if (Imm16::isInSignedRange(imm.value)) {
> 
> // Same as above

Thanks for catching these. Since this patch has already landed, I will add this to a separate patch.
Attached patch MoveEmitter-mips.patch (obsolete) — Splinter Review
Attachment #8382987 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8382987 [details] [diff] [review]
MoveEmitter-mips.patch

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

Modify this patch to use as much as possible the high level macro assembler instructions, such as somebody who does not know anything about MIPS can compare this code to other implementation without having to deal with the inverted logic of the "ma_l*" and "ma_s*" abstractions.

::: js/src/jit/mips/MoveEmitter-mips.cpp
@@ +69,5 @@
> +{
> +    // We will not use spill reg. Ve will use Macroassembler's second scratch
> +    // and be carefull not to use macro instructions that clobber it.
> +    spilledReg_ = t8;
> +    return t8;

Move the secondScratchReg_ outside the MacroAssembler, move it to the Assembler-mips.h, rename it to SecondScratchReg, and use it here instead of using t8.
And remove the comment.

@@ +86,5 @@
> +      case MoveOp::FLOAT32:
> +        if (to.isMemory()) {
> +            FloatRegister temp = ScratchFloatReg;
> +            masm.ma_ls(temp, getAdjustedAddress(to));
> +            masm.ma_ss(temp, cycleSlot());

use masm.loadFloat32 and masm.storeFloat32.

@@ +95,5 @@
> +      case MoveOp::DOUBLE:
> +        if (to.isMemory()) {
> +            FloatRegister temp = ScratchFloatReg;
> +            masm.ma_ld(temp, getAdjustedAddress(to));
> +            masm.ma_sd(temp, cycleSlot());

same thing with loadDouble & storeDouble.

@@ +106,5 @@
> +        // an non-vfp value
> +        if (to.isMemory()) {
> +            Register temp = tempReg();
> +            masm.ma_lw(temp, getAdjustedAddress(to));
> +            masm.ma_sw(temp, cycleSlot());

same thing with loadPtr & storePtr.

@@ +132,5 @@
> +      case MoveOp::FLOAT32:
> +        if (to.isMemory()) {
> +            FloatRegister temp = ScratchFloatReg;
> +            masm.ma_ls(temp, cycleSlot());
> +            masm.ma_ss(temp, getAdjustedAddress(to));

idem for this function.

@@ +189,5 @@
> +    } else if (from.isEffectiveAddress()) {
> +        if (to.isGeneralReg()) {
> +            masm.ma_addu(to.reg(),  from.base(), Imm32(getAdjustedOffset(from)));
> +        } else if (to.isMemory()) {
> +            masm.ma_addu(tempReg(),  from.base(), Imm32(getAdjustedOffset(from)));

use masm.computeEffectiveAddress(getAdjustedAddress(from), tempReg());
Attachment #8382987 - Flags: review?(nicolas.b.pierron)
Attached patch Refactor-SecondScratchReg.patch (obsolete) — Splinter Review
Attachment #8383727 - Flags: review?(nicolas.b.pierron)
Attached patch MoveEmitter-mips.patch (obsolete) — Splinter Review
Attachment #8382987 - Attachment is obsolete: true
Attachment #8383728 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8383727 [details] [diff] [review]
Refactor-SecondScratchReg.patch

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

Remove the 2 lines from the patch, as they do not match the topic of this renaming patch, and you can carry over the review+ added on this patch.
If you move these 2 lines into a separated patch, I can also r+ these quickly.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +408,4 @@
>  void
>  MacroAssemblerMIPS::ma_or(Register rd, Register rs, Imm32 imm)
>  {
> +    if (Imm16::isInUnsignedRange(imm.value)) {

These changes should be part of a separated patch, otherwise the subject of this patch is miss-leading.

@@ +438,4 @@
>  void
>  MacroAssemblerMIPS::ma_xor(Register rd, Register rs, Imm32 imm)
>  {
> +    if (Imm16::isInUnsignedRange(imm.value)) {

same here.
Attachment #8383727 - Flags: review?(nicolas.b.pierron) → review+
Removed offtopic part of the patch.
Carry reviews from previous patch.
Attachment #8383727 - Attachment is obsolete: true
Attachment #8385325 - Flags: review+
Comment on attachment 8383728 [details] [diff] [review]
MoveEmitter-mips.patch

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

Adress the questions, and ask again for review.

::: js/src/jit/mips/MoveEmitter-mips.cpp
@@ +99,5 @@
> +            masm.storeDouble(to.floatReg(), cycleSlot());
> +        }
> +        break;
> +      case MoveOp::INT32:
> +      case MoveOp::GENERAL:

nit: MOZ_ASSERT(sizeof(uintptr_t) == sizeof(int32_t)); under the MoveOp::Int32 case.

@@ +245,5 @@
> +            // This should only be used when passing double parameter in a2,a3
> +            if(to.reg() == a2)
> +                masm.as_mfc1(a2, from.floatReg());
> +            else if(to.reg() == a3)
> +                masm.as_mfc1_Odd(a3, from.floatReg());

Wouldn't a Double overlap both a2 & a3?
Are you registering 2 moves when we have a double as argument?
Attachment #8383728 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8383728 [details] [diff] [review]
MoveEmitter-mips.patch

(In reply to Nicolas B. Pierron [:nbp] from comment #58)
> 
> Wouldn't a Double overlap both a2 & a3?
> Are you registering 2 moves when we have a double as argument?

I am registering 2 moves for a double register. You can see this at:
https://hg.mozilla.org/mozilla-central/file/5dc091b7e86f/js/src/jit/mips/MacroAssembler-mips.cpp#l2912

This is a case where single double value is stored in a2, a3 pair.

I can add a comment that explains it here.
Attachment #8383728 - Flags: review?(nicolas.b.pierron)
Separated offtopic part of the Refactor-SecondScratchReg.patch patch.
Attachment #8385332 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8385332 [details] [diff] [review]
Fixed-ma_or-and-ma_xor.patch

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

Thanks Heiher for noticing.
Attachment #8385332 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8383728 [details] [diff] [review]
MoveEmitter-mips.patch

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

Ok, with the additional assertions and the comment.
Attachment #8383728 - Flags: review?(nicolas.b.pierron) → review+
Carry review from previous patch.
Attachment #8383728 - Attachment is obsolete: true
Attachment #8385435 - Flags: review+
Attached patch BaselineCompiler-mips.patch (obsolete) — Splinter Review
Attachment #8386745 - Flags: review?(jdemooij)
Attached patch Trampoline-mips.patch (obsolete) — Splinter Review
Attachment #8386868 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8386868 [details] [diff] [review]
Trampoline-mips.patch

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

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +23,5 @@
> +
> +static_assert(sizeof(uintptr_t) == sizeof(uint32_t), "Not 64-bit clean.");
> +
> +// Size of buffer that holds non-volotile regs plus alignment slot.
> +static const uint32_t GENERAL_REGS_BUFF_SIZE = 10 * sizeof(uintptr_t);

nit: volotile -> volatile

@@ +25,5 @@
> +
> +// Size of buffer that holds non-volotile regs plus alignment slot.
> +static const uint32_t GENERAL_REGS_BUFF_SIZE = 10 * sizeof(uintptr_t);
> +
> +static const uint32_t FLOAT_REGS_BUFF_SIZE = 6 * sizeof(double);

Make data structures and do sizeof of these data structures instead of creating these constants.

@@ +69,5 @@
> + *  double f22;
> + *  double f24;
> + *  double f26;
> + *  double f28;
> + *  double f30;     <- sp points here while saving and restoring registers.

… which would also avoid this comment, which basically describe the structure layout on the stack.

Making a data structure ensure that there is no subtle off-by-1 error in the multipliers.

@@ +195,5 @@
> +
> +        masm.subPtr(Imm32(2 * sizeof(uintptr_t)), s0);
> +        masm.subPtr(Imm32(2 * sizeof(uintptr_t)), StackPointer);
> +
> +        //TODO: If we make stack to be always aligned, we can use l.d and s.d.

nit: Todo's are written like "// :TODO: (Bug ABCDEF) comment", where ABCDEF is the bug which is dealing into fixing this issue.

If you are using a struct, and if the compiler ensure the stack alignment on calls, you can assert that the sure does not cause any miss-alignment.

@@ +199,5 @@
> +        //TODO: If we make stack to be always aligned, we can use l.d and s.d.
> +        masm.loadPtr(Address(s0, sizeof(uintptr_t)), s1);
> +        masm.storePtr(s1, Address(StackPointer, sizeof(uintptr_t)));
> +        masm.loadPtr(Address(s0, 0), s1);
> +        masm.storePtr(s1, Address(StackPointer, 0));

Use loadValue and saveValue here, and make a temporary ValueOperand which is using both s6 ans s7.

@@ +222,5 @@
> +        regs.take(OsrFrameReg);
> +        regs.take(BaselineFrameReg);
> +        regs.take(reg_code);
> +
> +        const Address slot_numStackValues(BaselineFrameReg, NUM_STACK_VALUES_OFFSET);

use offsetof instead of this static const.

@@ +337,5 @@
> +    MacroAssembler masm(cx);
> +
> +    static const uint32_t STACK_DATA_SIZE = Registers::Total * sizeof(uintptr_t) +
> +                                            FloatRegisters::Total * sizeof(double) +
> +                                            2 * sizeof(uintptr_t);

sizeof(BailoutStack) ?

@@ +341,5 @@
> +                                            2 * sizeof(uintptr_t);
> +    static const uint32_t RETVAL_OFFSET = sizeof(uintptr_t);
> +    static const uint32_t INV_DATA_OFFSET = RETVAL_OFFSET + sizeof(uintptr_t);
> +    static const uint32_t GEN_REGS_OFFSET = INV_DATA_OFFSET +
> +                                            FloatRegisters::Total * sizeof(double);

use offsetof(…, …) instead.
Attachment #8386868 - Flags: review?(nicolas.b.pierron)
Attachment #8386745 - Flags: review?(nfroyd)
Comment on attachment 8386745 [details] [diff] [review]
BaselineCompiler-mips.patch

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

Looks fine.

::: js/src/jit/mips/BaselineCompiler-mips.h
@@ +11,5 @@
> +
> +namespace js {
> +namespace jit {
> +
> +class BaselineCompilerMIPS : public BaselineCompilerShared

Eventually we should remove the empty platform-dependent BaselineCompiler* classes, but that doesn't have to happen in this bug.

::: js/src/jit/mips/BaselineHelpers-mips.h
@@ +28,5 @@
> +
> +inline void
> +EmitRepushTailCallReg(MacroAssembler &masm)
> +{
> +    // No-op on MIPS because link register is always holding the return address.

Nit: s/link/ra, like you did in the previous function?
Attachment #8386745 - Flags: review?(jdemooij) → review+
Comment on attachment 8386745 [details] [diff] [review]
BaselineCompiler-mips.patch

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

::: js/src/jit/mips/BaselineHelpers-mips.h
@@ +101,5 @@
> +    MOZ_ASSERT(BaselineTailCallReg == ra);
> +    masm.makeFrameDescriptor(t6, IonFrame_BaselineJS);
> +    masm.subPtr(Imm32(2 * sizeof(intptr_t)), StackPointer);
> +    masm.storePtr(t6, Address(StackPointer, sizeof(intptr_t)));
> +    masm.storePtr(ra, Address(StackPointer, 0));

I feel like you might be able to make a small two-member structure here too, and use it in the tail call cases below.

@@ +128,5 @@
> +}
> +
> +// Size of vales pushed by EmitEnterStubFrame.
> +static const uint32_t STUB_FRAME_SIZE = 4 * sizeof(intptr_t);
> +static const uint32_t STUB_FRAME_SAVED_STUB_OFFSET = sizeof(intptr_t);

Similar to nbp's comments on the trampoline patch, you should have a data structure for the stub frame.  Then you can use sizeof and offsetof to figure out where everything goes.

@@ +147,5 @@
> +    // if needed.
> +
> +    // Push frame descriptor and return address.
> +    masm.makeFrameDescriptor(scratch, IonFrame_BaselineJS);
> +    masm.subPtr(Imm32(4 * sizeof(intptr_t)), StackPointer);

This should have been STUB_FRAME_SIZE, but would be the data structure sizeof with the changes above.

@@ +157,5 @@
> +    masm.storePtr(BaselineFrameReg, Address(StackPointer, 0));
> +    masm.movePtr(BaselineStackReg, BaselineFrameReg);
> +
> +    // We pushed 4 words, so the stack is still aligned to 8 bytes.
> +    masm.checkStackAlignment();

Presumably you could also make this static_assert on the size of the data structure instead of a runtime check.

@@ +183,5 @@
> +    masm.loadPtr(Address(StackPointer, 2 * sizeof(intptr_t)), BaselineTailCallReg);
> +
> +    // Discard the frame descriptor.
> +    masm.loadPtr(Address(StackPointer, 3 * sizeof(intptr_t)), ScratchRegister);
> +    masm.addPtr(Imm32(4 * sizeof(intptr_t)), StackPointer);

This should have been STUB_FRAME_SIZE, too.

@@ +209,5 @@
> +{
> +    MOZ_ASSERT(values >= 0 && values <= 2);
> +    switch(values) {
> +      case 1:
> +        // Unstow R0

Nit: periods end sentences.

@@ +216,5 @@
> +        else
> +            masm.popValue(R0);
> +        break;
> +      case 2:
> +        // Unstow R0 and R1

Nit: periods end sentences.

::: js/src/jit/mips/BaselineIC-mips.cpp
@@ +99,5 @@
> +        masm.ma_mul_branch_overflow(scratchReg, R0.payloadReg(), R1.payloadReg(), &failure);
> +
> +        masm.ma_b(scratchReg, Imm32(0), &goodMul, Assembler::NotEqual, ShortJump);
> +
> +        // Result is -0 if operands have differnet signs.

Typo: "different".

@@ +152,5 @@
> +        masm.ma_and(R0.payloadReg() , R0.payloadReg(), R1.payloadReg());
> +        break;
> +      case JSOP_LSH:
> +        // MIPS will happily try to shift by more than 0x1f.
> +        masm.ma_sll(R0.payloadReg(), R0.payloadReg(), R1.payloadReg());

Then you need to mask off the upper bits of R1 prior to shifting.  For the JSOP_RSH and JSOP_URSH cases, too.
Attachment #8386745 - Flags: review?(nfroyd) → feedback+
Attachment #8386745 - Attachment is obsolete: true
Attachment #8388490 - Flags: review?(nfroyd)
Attachment #8388490 - Flags: review?(jdemooij)
(In reply to Nathan Froyd (:froydnj) from comment #68)
> @@ +157,5 @@
> > +    masm.storePtr(BaselineFrameReg, Address(StackPointer, 0));
> > +    masm.movePtr(BaselineStackReg, BaselineFrameReg);
> > +
> > +    // We pushed 4 words, so the stack is still aligned to 8 bytes.
> > +    masm.checkStackAlignment();
> 
> Presumably you could also make this static_assert on the size of the data
> structure instead of a runtime check.

I think that this should stay a runtime check because we are also checking if the stack was properly aligned before the push.


> @@ +152,5 @@
> > +        masm.ma_and(R0.payloadReg() , R0.payloadReg(), R1.payloadReg());
> > +        break;
> > +      case JSOP_LSH:
> > +        // MIPS will happily try to shift by more than 0x1f.
> > +        masm.ma_sll(R0.payloadReg(), R0.payloadReg(), R1.payloadReg());
> 
> Then you need to mask off the upper bits of R1 prior to shifting.  For the
> JSOP_RSH and JSOP_URSH cases, too.

This comment was wrong. Here is quote from MIPS documentation:
The bit-shift amount is specified by the low-order 5 bits of GPR rs.
Comment on attachment 8388490 [details] [diff] [review]
BaselineCompiler-mips.patch

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

::: js/src/jit/mips/BaselineIC-mips.cpp
@@ +152,5 @@
> +        masm.ma_and(R0.payloadReg() , R0.payloadReg(), R1.payloadReg());
> +        break;
> +      case JSOP_LSH:
> +        // MIPS will only use 5 lowest bits in R1 as shift offset.
> +        masm.ma_sll(R0.payloadReg(), R0.payloadReg(), R1.payloadReg());

Ah, I failed to carefully consult the instruction definition here.  No masking needed!
Attachment #8388490 - Flags: review?(nfroyd) → review+
Attachment #8388490 - Flags: review?(jdemooij) → review+
Attached patch Trampoline-mips.patch (obsolete) — Splinter Review
Attachment #8386868 - Attachment is obsolete: true
Attachment #8389134 - Flags: review?(nicolas.b.pierron)
Attached patch Trampoline-mips.patch (obsolete) — Splinter Review
I just fixed a small issue in this file.
Attachment #8389134 - Attachment is obsolete: true
Attachment #8389134 - Flags: review?(nicolas.b.pierron)
Attachment #8389779 - Flags: review?(nicolas.b.pierron)
Attached patch Lowering-mips.patch (obsolete) — Splinter Review
Attachment #8390418 - Flags: review?(jdemooij)
Comment on attachment 8389779 [details] [diff] [review]
Trampoline-mips.patch

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

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +46,5 @@
> +    uintptr_t s2;
> +    uintptr_t s1;
> +    uintptr_t s0;
> +
> +    // First 4 argumet placeholders

Split this structure here …

@@ +60,5 @@
> +    Value *vp;
> +};
> +
> +// Size of buffer that holds non-volatile regs plus alignment slot.
> +static const uint32_t REGS_BUFF_SIZE = 10 * sizeof(uintptr_t) + 6 * sizeof(double);

… and use only one sizeof with no multiplications/additions here.

@@ +66,5 @@
> +static void
> +GenerateReturn(MacroAssembler &masm, int returnCode)
> +{
> +    // Restore non-volatile registers
> +    masm.loadPtr(Address(StackPointer, offsetof(EnterJITStack, s0)), s0);

Assert that masm.framePushed() == REGS_BUFF_SIZE

@@ +84,5 @@
> +    masm.loadDouble(Address(StackPointer, offsetof(EnterJITStack, f26)), f26);
> +    masm.loadDouble(Address(StackPointer, offsetof(EnterJITStack, f28)), f28);
> +    masm.loadDouble(Address(StackPointer, offsetof(EnterJITStack, f30)), f30);
> +
> +    masm.addPtr(Imm32(REGS_BUFF_SIZE), StackPointer);

Use masm.freeStack(REGS_BUFF_SIZE);

@@ +115,5 @@
> +
> +    // Save non-volatile registers. These must be saved by the trampoline,
> +    // rather than the JIT'd code, because they are scanned by the conservative
> +    // scanner.
> +    masm.subPtr(Imm32(REGS_BUFF_SIZE), StackPointer);

Use masm.reserveStack(REGS_BUFF_SIZE)

@@ +131,5 @@
> +    masm.as_sd(f22, StackPointer, offsetof(EnterJITStack, f22));
> +    masm.as_sd(f24, StackPointer, offsetof(EnterJITStack, f24));
> +    masm.as_sd(f26, StackPointer, offsetof(EnterJITStack, f26));
> +    masm.as_sd(f28, StackPointer, offsetof(EnterJITStack, f28));
> +    masm.as_sd(f30, StackPointer, offsetof(EnterJITStack, f30));

Create a function called GeneratePrologue to make a parallel with GenerateReturn, and move this code to this new function.

@@ +528,5 @@
> +{
> +    // NOTE: Members snapshotOffset_ and padding_ of BailoutStack
> +    // are not stored in this function.
> +    static const uint32_t BAILOUT_DATA_SIZE = sizeof(BailoutStack) - 2 * sizeof(uintptr_t);
> +    static const uint32_t AFTER_BAILOUT_SIZE = 2 * sizeof(uintptr_t);

AFTER_BAILOUT_SIZE should be renamed to sizeOfBailoutInfoOutParam or something similar.
Also, we do not need to allocate it on the stack from the beginning, if this can remove extra additions.

@@ +561,5 @@
> +
> +    // Put pointer to BailoutStack as first argument to the Bailout()
> +    masm.ma_addu(a0, StackPointer, Imm32(AFTER_BAILOUT_SIZE));
> +    // Put pointer to BailoutInfo
> +    masm.movePtr(StackPointer, a1);

You will need to reset the content of the memory targeted by a1, as this is asserted under jit::BailoutIonToBaseline.

@@ +701,5 @@
> +        // Double outparam has to be 8 byte aligned because the called
> +        // function can use sdc1 or ldc1 instructions to access it.
> +        masm.ma_and(outReg, StackPointer, Imm32(~(StackAlignment - 1)));
> +        masm.subPtr(Imm32(sizeof(double)), outReg);
> +        masm.reserveStack(3 * sizeof(uintptr_t));

nit:  masm.reserveStack(StackAlignment + sizeof(double));

@@ +785,5 @@
> +        masm.freeStack(sizeof(uintptr_t));
> +        break;
> +
> +      case Type_Double:
> +        masm.freeStack(3 * sizeof(uintptr_t));

Same here.
Attachment #8389779 - Flags: review?(nicolas.b.pierron)
Attached patch Trampoline-mips.patch (obsolete) — Splinter Review
Attachment #8389779 - Attachment is obsolete: true
Attachment #8391310 - Flags: review?(nicolas.b.pierron)
Attachment #8385325 - Flags: checkin+
Attachment #8385332 - Flags: checkin+
Attachment #8385435 - Flags: checkin+
Attachment #8388490 - Flags: checkin+
Comment on attachment 8391310 [details] [diff] [review]
Trampoline-mips.patch

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

Nice work.

::: js/src/jit/mips/Trampoline-mips.cpp
@@ +50,5 @@
> +
> +struct EnterJITArgs
> +{
> +    // First 4 argumet placeholders
> +    void * jitcode; // <- sp points here when function is entered.

nit: void *jitcode;
           ^ no space after the *.

@@ +133,5 @@
> +    const Register reg_argv = a2;
> +    const Register reg_frame = a3;
> +
> +    const Address slotToken(sp, sizeof(EnterJITRegs) + offsetof(EnterJITArgs, calleeToken));
> +    const Address slotVp(sp, sizeof(EnterJITRegs) + offsetof(EnterJITArgs, vp));

Move these 2 lines after GeneratePrologue.

@@ +443,5 @@
> +        masm.subPtr(Imm32(sizeof(Value)), StackPointer);
> +        masm.load32(Address(t2, sizeof(Value) / 2), t0);
> +        masm.store32(t0, Address(StackPointer, sizeof(Value) / 2));
> +        masm.load32(Address(t2, 0), t0);
> +        masm.store32(t0, Address(StackPointer, 0));

Use NUNBOX32_TYPE_OFFSET and NUNBOX32_PAYLOAD_OFFSET.
Attachment #8391310 - Flags: review?(nicolas.b.pierron) → review+
Attached patch Trampoline-mips.patch (obsolete) — Splinter Review
Carry review from the previous patch.
Attachment #8391310 - Attachment is obsolete: true
Attachment #8392212 - Flags: review+
Comment on attachment 8390418 [details] [diff] [review]
Lowering-mips.patch

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

Sorry for the delay.

::: js/src/jit/mips/LIR-mips.h
@@ +401,5 @@
> +} // namespace jit
> +} // namespace js
> +
> +#endif /* jit_mips_LIR_mips_h */
> +

Nit: remove this empty line.

::: js/src/jit/mips/LOpcodes-mips.h
@@ +25,5 @@
> +    _(AsmJSLoadFuncPtr)
> +
> +
> +#endif // jit_mips_LOpcodes_mips_h__
> +

Same here.
Attachment #8390418 - Flags: review?(jdemooij) → review+
I fixed the nits.
Carry review from previous patch.

(In reply to Jan de Mooij [:jandem] from comment #81)
> Sorry for the delay.

No problem.
Attachment #8390418 - Attachment is obsolete: true
Attachment #8392905 - Flags: review+
Attached patch 0001-Architecture-mips.patch (obsolete) — Splinter Review
Update MIPS code for upstreaming.

js/src/jit/mips/Architecture-mips.h
js/src/jit/mips/Architecture-mips.cpp
Attached patch 0002-Architecture-mips.patch (obsolete) — Splinter Review
Add MIPS N32 ABI support.

js/src/jit/mips/Architecture-mips.h
js/src/jit/mips/Architecture-mips.cpp
Depends on: 985876
Blocks: 985881
Heiher has used my code to create a MIPS n32 port. He has finished this work with some help from me. I had a chance to take a look at his code and it looks like good work to me.

Heiher, since this bug is large enough as it is i would preffer if you used a separate bug to upstream your code. I have created bug 985881 for this purpose.
(In reply to Branislav Rankov from comment #85)
> Heiher has used my code to create a MIPS n32 port. He has finished this work
> with some help from me. I had a chance to take a look at his code and it
> looks like good work to me.
> 
> Heiher, since this bug is large enough as it is i would preffer if you used
> a separate bug to upstream your code. I have created bug 985881 for this
> purpose.

OK, Thank you very much!
Attached patch CodeGenerator-mips.patch (obsolete) — Splinter Review
Attachment #8393920 - Attachment is obsolete: true
Attachment #8393921 - Attachment is obsolete: true
Attachment #8404013 - Flags: review?(nicolas.b.pierron)
Attachment #8404013 - Flags: review?(nfroyd)
Attachment #8392212 - Flags: checkin?(nicolas.b.pierron)
Attachment #8392905 - Flags: checkin?(nicolas.b.pierron)
Blocks: 994716
Comment on attachment 8404013 [details] [diff] [review]
CodeGenerator-mips.patch

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

I am really sad that we have so much duplication between ARM and MIPS, I think that if we use higher level of abstraction of the MacroAssembler, we would be able to progressively share more and more code, and as well catch more hidden bugs that are hard to catch with large reviews.

The current patch is ok, except for the fact that it relies to much on internal macro assembler functions.

::: js/src/jit/mips/CodeGenerator-mips.cpp
@@ +229,5 @@
> +bool
> +CodeGeneratorMIPS::bailout(LSnapshot *snapshot)
> +{
> +    Label label;
> +    masm.ma_b(&label);

We should use the generic macro assembler instead of architecture specific one, unless we are unable to express something.
Replace masm.ma_b by masm.jump, and same thing for all other CodeGen functions.

@@ +1975,5 @@
> +
> +bool
> +CodeGeneratorMIPS::visitLoadTypedArrayElementStatic(LLoadTypedArrayElementStatic *ins)
> +{
> +    MOZ_ASSUME_UNREACHABLE("NYI");

Any reasons to mark these as NYI?
Is that disabled in the lowering too?
Attachment #8404013 - Flags: review?(nicolas.b.pierron)
Attachment #8404013 - Flags: review-
Attachment #8404013 - Flags: feedback+
Attached patch MIPS-includes-and-checks.patch (obsolete) — Splinter Review
(In reply to Nicolas B. Pierron [:nbp] from comment #88)
> 
> I am really sad that we have so much duplication between ARM and MIPS, I
> think that if we use higher level of abstraction of the MacroAssembler, we
> would be able to progressively share more and more code, and as well catch
> more hidden bugs that are hard to catch with large reviews.
> 
> The current patch is ok, except for the fact that it relies to much on
> internal macro assembler functions.

I agree that we should try to share as much code as possible between ARM and MIPS. There might be some duplication with X86 also. The biggest obstacle for MIPS currently are the ShorJump flags. If we added this flag to common branch instructions, we could refactor to share good part of the code between MIPS and ARM. I'm not sure if X86, X64 and ARM can benefit from ShortJump flag. 

I will update code and identify what other issues exist that prevent sharing code.

> > +bool
> > +CodeGeneratorMIPS::visitLoadTypedArrayElementStatic(LLoadTypedArrayElementStatic *ins)
> > +{
> > +    MOZ_ASSUME_UNREACHABLE("NYI");
> 
> Any reasons to mark these as NYI?
> Is that disabled in the lowering too?

This has NYI in Lowering too. ARM has it the same way also. It didn't come up in any of the test cases. So I assumed that it is no longer used.
Comment on attachment 8404013 [details] [diff] [review]
CodeGenerator-mips.patch

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

I don't see a lot of MIPS-specific stuff in here, so I'm going to defer to Nicolas's review.
Attachment #8404013 - Flags: review?(nfroyd)
> I am really sad that we have so much duplication between ARM and MIPS

Then you'll be even sadder when I finally finish the PowerPC port; even PPC Baseline Compiler has an awful lot of similarities.
Attachment #8405417 - Flags: review?(jdemooij)
Attached patch CodeGenerator-mips.patch (obsolete) — Splinter Review
I have found some more instructions that have a common fuction and fixed them. Here is the list of functions that need to be added to common interface so that MIPS and ARM have more compatible code:

- All branches need the ShortJump flag.

- The following functions need variants with 3 parameters (separate output register):
add32
sub32
branchSub32
branchAdd32
addPtr
subPtr
lshift32
rshift32
xor32
and32
addDouble
subDouble
mulDouble
divDouble

- We need rshift32 in both arithmetic and logical variant.

- Need to add:
addFloat32
subFloat32
mulFloat32
divFloat32

- The following functions need variants with 2 parameters (separate output register):
neg32
not32
negateDouble

- The folowing can go either way, they can stay architecture specific, or they can be made common:
branchMul (mips has ma_mul_branch_overflow)
branchDiv (mips has ma_div_branch_overflow)
ma_mod_mask (both mips and arm have these)
cmpDoubleSet (mips has ma_cmp_set_double)
cmpFloat32Set (mips has ma_cmp_set_float32)

- Consider adding:
sqrtDouble
sqrtFloat32
absDouble
absFloat32
negateFloat32

I propose that we create a bug to make these modifications (or some of them) and modify this code once this is done.
Attachment #8404013 - Attachment is obsolete: true
Attachment #8406155 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8405417 [details] [diff] [review]
MIPS-includes-and-checks.patch

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

Looks good, r=me with comments below addressed.

::: js/src/jit/BaselineBailouts.cpp
@@ +348,1 @@
>          // On X64 and ARM, the frame pointer save location depends on the caller of the

Nit: On x64, ARM and MIPS, the frame pointer...

::: js/src/jit/Registers.h
@@ +18,5 @@
>  # include "jit/arm/Architecture-arm.h"
> +#elif defined(JS_CODEGEN_MIPS)
> +# include "jit/mips/Architecture-mips.h"
> +#else
> +# error "Unknown architecture!"

Thanks for adding these #errors, should make life a bit easier for other ports.

::: js/src/jit/shared/Assembler-x86-shared.cpp
@@ +12,5 @@
>  # include "jit/x64/MacroAssembler-x64.h"
>  #elif defined(JS_CODEGEN_ARM)
>  # include "jit/arm/MacroAssembler-arm.h"
> +#elif defined(JS_CODEGEN_MIPS)
> +# include "jit/mips/MacroAssembler-mips.h"

We are in Assembler-x86-shared.cpp, so it seems you could just remove the ARM and MIPS includes here right?

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +65,5 @@
>  
>          // An MAsmJSCall does not align the stack pointer at calls sites but instead
>          // relies on the a priori stack adjustment (in the prologue) on platforms
>          // (like x64) which require the stack to be aligned.
> +#if defined JS_CODEGEN_ARM || defined JS_CODEGEN_MIPS

Add parentheses around JS_CODEGEN_ARM and JS_CODEGEN_MIPS:

#if defined(JS_CODEGEN_ARM) || ...

::: js/src/jit/shared/MoveEmitter-x86-shared.h
@@ +14,5 @@
>  #elif defined(JS_CODEGEN_ARM)
>  # include "jit/arm/MacroAssembler-arm.h"
> +#elif defined(JS_CODEGEN_MIPS)
> +# include "jit/mips/MacroAssembler-mips.h"
> +#else

This is MoveEmitter-x86-shared, so just remove both the ARM and MIPS includes (but leave the #error).
Attachment #8405417 - Flags: review?(jdemooij) → review+
Carry review from previous patch.

I fixed all the commented issues.

(In reply to Jan de Mooij [:jandem] from comment #94)
> This is MoveEmitter-x86-shared, so just remove both the ARM and MIPS
> includes (but leave the #error).

I also changed the error to read:
Wrong architecture. Only x86 and x64 should build this file!
Attachment #8405417 - Attachment is obsolete: true
Attachment #8406744 - Flags: review+
Blocks: 996561
Attached patch CodeGenerator-mips.patch (obsolete) — Splinter Review
Updated visitDivI with changes from bug 995817.
Attachment #8406155 - Attachment is obsolete: true
Attachment #8406155 - Flags: review?(nicolas.b.pierron)
Attachment #8408246 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8408246 [details] [diff] [review]
CodeGenerator-mips.patch

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

I think this is good to go, I have not noticed any critical issues in the patch.

We should make follow-up patches for some of the nits, as indicated.

::: js/src/jit/mips/CodeGenerator-mips.cpp
@@ +796,5 @@
> +        masm.ma_mod_mask(src, dest, tmp, ins->shift(), &bail);
> +        if (!bailoutFrom(&bail, ins->snapshot()))
> +            return false;
> +    } else {
> +        masm.ma_mod_mask(src, dest, tmp, ins->shift(), nullptr);

followup-nit: all other ma_* functions seems to have the destination as first argument, except this one.

@@ +1194,5 @@
> +    masm.loadConstantDouble(0.5, temp);
> +
> +    // Branch to a slow path for negative inputs. Doesn't catch NaN or -0.
> +    masm.loadConstantDouble(0.0, scratch);
> +    masm.ma_bc1d(input, scratch, &negative, Assembler::DoubleLessThan, ShortJump);

follow-up nits: same thing as ma_mod_mask.

@@ +1378,5 @@
> +
> +bool
> +CodeGeneratorMIPS::visitValue(LValue *value)
> +{
> +    const ValueOperand out = ToOutValue(value);

follow-up: This function should be moved in CodeGenerator.cpp

@@ +1393,5 @@
> +    MOZ_ASSERT(!box->getOperand(0)->isConstant());
> +
> +    // On x86, the input operand and the output payload have the same
> +    // virtual register. All that needs to be written is the type tag for
> +    // the type definition.

I agree with x86, but this seems to be NUNBOX32 related more than x86 / ARM / MIPS ;)

@@ +1450,5 @@
> +
> +Register
> +CodeGeneratorMIPS::splitTagForTest(const ValueOperand &value)
> +{
> +    return value.typeReg();

Ok, this is weird, I admit that ARM is doing the same thing, but on x86 we have it as part of the Macro Assembler, which makes more sense to me.
No need to change everything now, but we should probably unify that for NUNBOX32 targets.

@@ +1672,5 @@
> +        masm.ma_and(ScratchRegister, ToRegister(baab->left()), Imm32(ToInt32(baab->right())));
> +    else
> +        masm.ma_and(ScratchRegister, ToRegister(baab->left()), ToRegister(baab->right()));
> +    emitBranch(ScratchRegister, ScratchRegister, Assembler::NonZero, baab->ifTrue(),
> +               baab->ifFalse());

nit: I do not think "baab" brings more value compared to "lir", even if this is funny to read. :)

@@ +2001,5 @@
> +      case ArrayBufferView::TYPE_INT16:   isSigned = true;  size = 16; break;
> +      case ArrayBufferView::TYPE_UINT16:  isSigned = false; size = 16; break;
> +      case ArrayBufferView::TYPE_INT32:   isSigned = true;  size = 32; break;
> +      case ArrayBufferView::TYPE_UINT32:  isSigned = false;  size = 32; break;
> +      case ArrayBufferView::TYPE_FLOAT64: isFloat = true;   size = 64; break;

nit: fix alignment of the above lines.

@@ +2086,5 @@
> +      case ArrayBufferView::TYPE_UINT8:   isSigned = false; size = 8; break;
> +      case ArrayBufferView::TYPE_INT16:
> +      case ArrayBufferView::TYPE_UINT16:  isSigned = false; size = 16; break;
> +      case ArrayBufferView::TYPE_INT32:
> +      case ArrayBufferView::TYPE_UINT32:  isSigned = true;  size = 32; break;

This enumeration sounds incorrect.
Attachment #8408246 - Flags: review?(nicolas.b.pierron) → review+
Carry review from previous patch.

I have fixed the nit's. The follow-up nit's will be done in a separate patch.
Attachment #8408246 - Attachment is obsolete: true
Attachment #8410262 - Flags: review+
Attachment #8392212 - Flags: checkin?(nicolas.b.pierron) → checkin+
Attachment #8392905 - Flags: checkin?(nicolas.b.pierron) → checkin+
(In reply to Carsten Book [:Tomcat] from comment #100)
> (In reply to Carsten Book [:Tomcat] from comment #99)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/ac10d2fcb25d
> 
> had to backout this changeset for test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38384706&tree=Mozilla-
> Inbound

Sorry about this. The include file has changed name. I made a separate patch for this. I should have updated this one. I will fix this.
I merged part of the patch 05-General-updates.patch from bug 994716 into this patch because of style check failures.

Both patches have r+, so I'm carrying the review.
Attachment #8392212 - Attachment is obsolete: true
Attachment #8411720 - Flags: review+
Attachment #8392905 - Flags: checkin+ → checkin?(nicolas.b.pierron)
Attachment #8406744 - Flags: checkin?(nicolas.b.pierron)
Attachment #8410262 - Flags: checkin?(nicolas.b.pierron)
Attachment #8411720 - Flags: checkin?(nicolas.b.pierron)
(In reply to Carsten Book [:Tomcat] from comment #99)
attachment Attachment #8392905 [details] [diff] landed already as, so setting the flag again :)

> https://hg.mozilla.org/integration/mozilla-inbound/rev/b035f5a9c20e
Attachment #8406744 - Flags: checkin?(nicolas.b.pierron) → checkin+
Attachment #8392905 - Flags: checkin?(nicolas.b.pierron) → checkin+
Attachment #8410262 - Flags: checkin?(nicolas.b.pierron) → checkin+
Attachment #8411720 - Flags: checkin?(nicolas.b.pierron) → checkin+
Blocks: 1001346
Blocks: 1007156
Attached patch EnableIonMonkey.patch (obsolete) — Splinter Review
Attachment #8465471 - Flags: review?(jdemooij)
Comment on attachment 8465471 [details] [diff] [review]
EnableIonMonkey.patch

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

\o/ Looks good.

This touches the build system though, so it requires review from a build peer. I'll request review from glandium as well, just to be sure, but the changes look straight-forward.
Attachment #8465471 - Flags: review?(mh+mozilla)
Attachment #8465471 - Flags: review?(jdemooij)
Attachment #8465471 - Flags: review+
Comment on attachment 8465471 [details] [diff] [review]
EnableIonMonkey.patch

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

::: js/src/configure.in
@@ +3169,5 @@
>      JS_ARM_SIMULATOR= )
> +MOZ_ARG_ENABLE_BOOL(mips-simulator,
> +[  --enable-mips-simulator Enable MIPS simulator for JIT code],
> +    JS_MIPS_SIMULATOR=1,
> +    JS_MIPS_SIMULATOR= )

Can you build with both simulators? If yes, then the code below needs to reflect that. If no, then the code above needs to reflect that.
Attachment #8465471 - Flags: review?(mh+mozilla) → review-
Attached patch EnableIonMonkey.patch (obsolete) — Splinter Review
Both simulators cannot be used together, so I added an error message for this case.
Attachment #8465471 - Attachment is obsolete: true
Attachment #8466134 - Flags: review?(mh+mozilla)
Comment on attachment 8466134 [details] [diff] [review]
EnableIonMonkey.patch

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

::: js/src/configure.in
@@ +3172,5 @@
> +    JS_MIPS_SIMULATOR=1,
> +    JS_MIPS_SIMULATOR= )
> +
> +if test -n "$JS_ARM_SIMULATOR" && test -n "$JS_MIPS_SIMULATOR"; then
> +    AC_MSG_ERROR([ARM and MIPS simulators cannot be enabled together.])

I'd rather explicit the conflicting flags.
Attachment #8466134 - Flags: review?(mh+mozilla) → review+
Attached patch EnableIonMonkey.patch (obsolete) — Splinter Review
Updated the error message. Carry review from previous patch.
Attachment #8479081 - Flags: review+
Forgot to make the previous patch obsolete.
Attachment #8466134 - Attachment is obsolete: true
Attachment #8479081 - Attachment is obsolete: true
Attachment #8479084 - Flags: review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/9287adaf50e3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Is there a need for manual testing on this?
Flags: needinfo?(branislav.rankov)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #115)
> Is there a need for manual testing on this?

I would say no. There are automated tests that can be run. These tests are enough to test Spidermonkey. They probably won't be run on regular basis on tbpl, but there is a plan to make them available for try.
Flags: needinfo?(branislav.rankov)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: