IonMonkey MIPS: Add MIPS32 instruction simulator for JIT code

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rankov, Assigned: svetozar.janjic)

Tracking

Trunk
mozilla32
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

Reporter

Description

5 years ago
Adapt MIPS32 instruction simulator from v8 to be used with IonMonkey JIT. Similar work has been done for ARM simulator in bug 959597.
Reporter

Comment 1

5 years ago
Svetozar is a member of the team that ported IonMonkey JIT to MIPS. He will handle upstreaming of MIPS instruction simulator code.
Assignee: nobody → svetozar.janjic
Status: NEW → ASSIGNED
Reporter

Updated

5 years ago
Depends on: 969375
Just for the record, I am willing to review the MIPS bits of this patch.
Assignee

Comment 3

5 years ago
Posted patch Simulator-mips.patch (obsolete) — Splinter Review
Attachment #8407412 - Flags: review?(nfroyd)
Assignee

Comment 4

5 years ago
Reporter

Updated

5 years ago
Attachment #8407412 - Flags: review?(nfroyd)
Attachment #8407412 - Flags: review?(jdemooij)
Reporter

Updated

5 years ago
Attachment #8407523 - Flags: review?(jdemooij)
Comment on attachment 8407412 [details] [diff] [review]
Simulator-mips.patch

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

I didn't go over the instructions details with a fine-toothed comb, nor did I look deeply at the simulator interface.  Jan is better equipped to deal with the latter.

::: js/src/jit/mips/Simulator-mips.cpp
@@ +578,5 @@
> +    js_delete(srt);
> +}
> +
> +// This macro provides a platform independent use of sscanf.
> +#define sscanF sscanf  // NOLINT

What's the rationale behind this?  Does some linter warn that you #include'd sscanf and you're not using it?

@@ +767,5 @@
> +    printAllRegs();
> +
> +    printf("\n\n");
> +    // f0, f1, f2, ... f31.
> +    printf("%3s,%3s: 0x%08x%08x %16.4e\n", FPU_REG_INFO(0) );

for (uint32_t i = 0; i < ...; i += 2) {
  printf("%3s,%3s: 0x%08x%08x %16.4e\n", FPU_REG_INFO(i));
}

?

@@ +846,5 @@
> +    sprintf(hexbytes, "0x%x 0x%x 0x%x 0x%x", bytes[0], bytes[1], bytes[2], bytes[3]);
> +    char llvmcmd[1024];
> +    sprintf(llvmcmd, "bash -c \"echo -n '%p'; echo '%s' | "
> +            "llvm-mc -disassemble -arch=mipsel -mcpu=mips32r2 | "
> +            "grep -v pure_instructions | grep -v .text\"", reinterpret_cast<void*>(pc), hexbytes);

Nit: you could just static_cast<void*>(bytes) here so a reader doesn't think you're doing anything unusual with pc.

@@ +1282,5 @@
> +
> +    // Allocate 2MB for the stack. Note that we will only use 1MB, see also
> +    // Simulator::stackLimit().
> +    static const size_t stackSize = 2 * 1024 * 1024;
> +    stack_ = reinterpret_cast<char*>(js_malloc(stackSize));

static_cast is fine here, I believe.

@@ +1477,5 @@
> +    // Read the bits from the unsigned integer register_[] array
> +    // into the double precision floating point value and return it.
> +    char buffer[2 * sizeof(registers_[0])];
> +    memcpy(buffer, &registers_[reg], 2 * sizeof(registers_[0]));
> +    memcpy(&dm_val, buffer, 2 * sizeof(registers_[0]));

Just:

memcpy(&dm_val, &registers_[reg], sizeof(dm_val));

instead of all this stuff with a temporary buffer.

@@ +1494,5 @@
> +{
> +    MOZ_ASSERT((fpureg >= 0) && (fpureg < Simulator::FPURegister::kNumFPURegisters)
> +           && ((fpureg % 2) == 0));
> +    return *mozilla::BitwiseCast<int64_t*>(
> +               const_cast<int32_t*>(&FPUregisters_[fpureg]));

I'd argue that BitwiseCast should be reinterpret_cast here.

But what I'd really worry about here and the functions below (and their set* counterparts) is endianness.  I guess you're assuming that your simulated MIPS always has host endianness?  (I think we discussed this before, but I can't remember what the verdict was.)

@@ +1674,5 @@
> +    }
> +    printf("Unaligned (double) read at 0x%08x, pc=0x%08x\n",
> +           addr,
> +           reinterpret_cast<intptr_t>(instr));
> +    MOZ_CRASH();

Why the inconsistency in error behavior between unaligned reads/writes of non-words and unaligned reads/writes of words?  I'd think you'd want to drop into the debugger all the time.

@@ +1752,5 @@
> +uint32_t
> +Simulator::readBU(int32_t addr)
> +{
> +    uint8_t *ptr = reinterpret_cast<uint8_t*>(addr);
> +    return *ptr & 0xff;

& 0xff is unnecessary here.

@@ +1854,5 @@
> +
> +        int32_t *stack_pointer = reinterpret_cast<int32_t*>(getRegister(sp));
> +        // Args 4 and 5 are on the stack after the reserved space for args 0..3.
> +        int32_t arg4 = stack_pointer[4];
> +        int32_t arg5 = stack_pointer[5];

This is all ABI-specific, right?  You should really guard such things with preprocessor checks.

@@ +2469,5 @@
> +              case ff_div_fmt:
> +                setFpuRegisterFloat(fd_reg, fs_value / ft_value);
> +                break;
> +              case ff_abs_fmt:
> +                setFpuRegisterFloat(fd_reg, fabs(fs_value));

fabsf?

@@ +2478,5 @@
> +              case ff_neg_fmt:
> +                setFpuRegisterFloat(fd_reg, -fs_value);
> +                break;
> +              case ff_sqrt_fmt:
> +                setFpuRegisterFloat(fd_reg, sqrt(fs_value));

sqrtf?

@@ +2481,5 @@
> +              case ff_sqrt_fmt:
> +                setFpuRegisterFloat(fd_reg, sqrt(fs_value));
> +                break;
> +              case ff_c_un_fmt:
> +                setFCSRBit(fcsr_cc, std::isnan(fs_value) || std::isnan(ft_value));

FWIW, we have had problems with other formulations of isnan and isfinite; you may want to use the versions in mozilla/FloatingPoint.h.

@@ +2554,5 @@
> +                }
> +                break;
> +              }
> +              case ff_cvt_l_fmt: {  // Mips32r2: Truncate float to 64-bit long-word.
> +                float rounded = trunc(fs_value);

truncf?  (Here and below.)

::: js/src/jit/mips/Simulator-mips.h
@@ +136,5 @@
> +    friend class Redirection;
> +    friend class MipsDebugger;
> +public:
> +
> +    // Registers are declared in order. See SMRL chapter 2.

What is SMRL?  Please use the full name of the reference.

@@ +154,5 @@
> +        ra,
> +        // LO, HI, and pc.
> +        LO,
> +        HI,
> +        pc,   // pc must be the last register.

I think static_asserting this would be better, something like:

static_assert(pc == kPCRegister, ...);
static_assert(kNumSimuRegisters == (kPCRegister + 1), ...);
static_assert(kNumSimuRegisters == /* whatever constant we define outside the enum scope */, ...);

I notice that you've used kNumSimuRegisters for both the enum and the |const| variable above.  Perhaps you should get rid of the const variable and just use the enum value.  (Same goes for the PC, I guess, though perhaps the longer name for the constant is helpful.)

@@ +161,5 @@
> +        fp = s8
> +    };
> +
> +    // Coprocessor registers.
> +    // Generated code will always use doubles. So we will only use even registers.

This comment is going to be out of date very quickly.  Please remove it.

@@ +164,5 @@
> +    // Coprocessor registers.
> +    // Generated code will always use doubles. So we will only use even registers.
> +    enum FPURegister {
> +        f0, f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11,
> +        f12, f13, f14, f15,   // f12 and f14 are arguments FPURegisters.

I don't think this comment belongs here.

@@ +253,5 @@
> +    // Unsupported instructions use Format to print an error and stop execution.
> +    void format(SimInstruction* instr, const char* format);
> +
> +    // Read and write memory.
> +    inline uint32_t readBU(int32_t addr);

Shouldn't |addr| arguments to all of these be uint32_t or uintptr_t?

@@ +337,5 @@
> +    void callInternal(uint8_t *entry);
> +
> +    // Architecture state.
> +    // Registers.
> +    int32_t registers_[kNumSimuRegisters];

These should be uintptr_t, I should think.

@@ +339,5 @@
> +    // Architecture state.
> +    // Registers.
> +    int32_t registers_[kNumSimuRegisters];
> +    // Coprocessor Registers.
> +    int32_t FPUregisters_[kNumFPURegisters];

I'd think these should be uint64_t, with checks for 32-bit FPUs scattered in the appropriate places.

@@ +349,5 @@
> +    bool pc_modified_;
> +    int icount_;
> +    int break_count_;
> +
> +    int32_t resume_pc_;

uint32_t/uintptr_t?
Attachment #8407412 - Flags: review?(nfroyd) → feedback+
Attachment #8407523 - Flags: review?(jdemooij) → review+
Comment on attachment 8407412 [details] [diff] [review]
Simulator-mips.patch

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

This looks great. Good that it uses the same interface as the ARM simulator so that it requires minimal changes to the rest of the VM.

I didn't look too closely at the actual simulator as nfroyd already gave feedback and it's mostly imported from V8. Comments below are mainly style nits; if you can post an updated patch I'll take another look.

Also, please upload a separate patch to add "js/src/jit/mips" after "js/src/jit/arm" in toolkit/content/license.html (for about:license) and request review from :gerv

::: js/src/jit/mips/Simulator-mips.cpp
@@ +44,5 @@
> +
> +static const Instr kCallRedirInstr = op_special | MAX_BREAK_CODE << RTShift | ff_break;
> +
> +// Utils functions.
> +bool HaveSameSign(int32_t a, int32_t b)

static bool
HaveSameSign(...)

@@ +50,5 @@
> +    return ((a ^ b) >= 0);
> +}
> +
> +uint32_t GetFCSRConditionBit(uint32_t cc)
> +{

static uint32_t
GetFCSR...
{
    if (cc == 0)
        return 23;
    return 24 + cc;
}

@@ +109,5 @@
> +
> +    // Accessors for the different named fields used in the MIPS encoding.
> +    inline Opcode opcodeValue() const {
> +        return static_cast<Opcode>(
> +                   bits(OpcodeShift + OpcodeBits - 1, OpcodeShift));

99 chars per line for code so this should fit on one line.

@@ +456,5 @@
> +const int kBranchReturnOffset = 2 * SimInstruction::kInstrSize;
> +
> +class Redirection;
> +
> +class SimulatorRuntime

This duplicates some code from the ARM simulator. It's not a huge amount of code and it's nicer to have the definition in the cpp files than in some shared header file, so I think this is fine.

@@ +470,5 @@
> +        static HashNumber hash(const Lookup &l);
> +        static bool match(const Key &k, const Lookup &l);
> +    };
> +
> +public:

Indent with 2 spaces. Please search for public:, private: and protected: in this file and fix the others too.

@@ +578,5 @@
> +    js_delete(srt);
> +}
> +
> +// This macro provides a platform independent use of sscanf.
> +#define sscanF sscanf  // NOLINT

Remove this define and just use sscanf below instead of sscanF. V8 has its own sscanf implementation or something, but since this only affects the simulator's debugger let's just use sscanf directly for now; we can fix if it's a problem somewhere.

@@ +586,5 @@
> +class MipsDebugger
> +{
> +public:
> +    explicit MipsDebugger(Simulator *sim) : sim_(sim) { }
> +    ~MipsDebugger();

Remove the empty destructor.

@@ +622,5 @@
> +MipsDebugger::~MipsDebugger()
> +{
> +}
> +
> +#define UNSUPPORTED() printf("Unsupported instruction.\n");

static void
UNSUPPORTED()
{
    printf("Unsupported instruction.\n");
    MOZ_CRASH();
}

Compilers will inline this trivial function, and the MOZ_CRASH() is nice when debugging (to get a stack trace).

@@ +698,5 @@
> +{
> +    // Check if a breakpoint can be set. If not return without any side-effects.
> +    if (sim_->break_pc_ != nullptr) {
> +        return false;
> +    }

Nit: no {}

@@ +1315,5 @@
> +    registers_[ra] = bad_ra;
> +
> +    for (int i = 0; i < kNumExceptions; i++) {
> +        exceptions[i] = 0;
> +    }

Nit: no {}

@@ +1464,5 @@
> +    MOZ_ASSERT((reg >= 0) && (reg < Register::kNumSimuRegisters));
> +    if (reg == 0)
> +        return 0;
> +    else
> +        return registers_[reg] + ((reg == pc) ? SimInstruction::kPCReadOffset : 0);

Nit: SpiderMonkey style is to remove the |else| in this case.

@@ +1502,5 @@
> +Simulator::getFpuRegisterFloat(int fpureg) const
> +{
> +    MOZ_ASSERT((fpureg >= 0) && (fpureg < Simulator::FPURegister::kNumFPURegisters));
> +    return *mozilla::BitwiseCast<float*>(
> +               const_cast<int32_t*>(&FPUregisters_[fpureg]));

This fits on one line.

@@ +1551,5 @@
> +    if (value) {
> +        FCSR_ |= (1 << cc);
> +    } else {
> +        FCSR_ &= ~(1 << cc);
> +    }

Nit: no {}

@@ +2072,5 @@
> +Simulator::enableStop(uint32_t code)
> +{
> +    if (!isEnabledStop(code)) {
> +        watchedStops_[code].count_ &= ~kStopDisabledBit;
> +    }

Nit: no {}

@@ +2080,5 @@
> +Simulator::disableStop(uint32_t code)
> +{
> +    if (isEnabledStop(code)) {
> +        watchedStops_[code].count_ |= kStopDisabledBit;
> +    }

Nit: no {}

@@ +2128,5 @@
> +{
> +    for (int i = 1; i < kNumExceptions; i++) {
> +        if (exceptions[i] != 0) {
> +            MOZ_ASSUME_UNREACHABLE("Error: Exception raised.");
> +            //V8_Fatal(__FILE__, __LINE__, "Error: Exception %i raised.", i);

Nit: remove this line.

@@ +2136,5 @@
> +
> +// Handle execution based on instruction types.
> +void
> +Simulator::configureTypeRegister(SimInstruction *instr,
> +                                      int32_t& alu_out,

Nit: fix indentation of this argument and the ones below to match the first argument.

@@ +2665,5 @@
> +                if (setFCSRRoundError(ds_value, rounded)) {
> +                    setFpuRegister(fd_reg, kFPUInvalidResult);
> +                }
> +              }
> +              break;

Put the "break;" before the } to match some other cases, also a few times below.

@@ +3226,5 @@
> +
> +    // If needed update pc after the branch delay execution.
> +    if (next_pc != bad_ra) {
> +        set_pc(next_pc);
> +    }

Nit: no {}

@@ +3251,5 @@
> +    // Update pc and ra if necessary.
> +    // Do this after the branch delay execution.
> +    if (instr->isLinkingInstruction()) {
> +        setRegister(31, current_pc + 2 * SimInstruction::kInstrSize);
> +    }

Same here.

@@ +3281,5 @@
> +        UNSUPPORTED();
> +    }
> +    if (!pc_modified_) {
> +        setRegister(pc, reinterpret_cast<int32_t>(instr) +
> +                     SimInstruction::kInstrSize);

This fits on one line (and drop the {})

::: js/src/jit/mips/Simulator-mips.h
@@ +99,5 @@
> +// -----------------------------------------------------------------------------
> +// Utility functions
> +
> +class CachePage {
> +public:

You can move CachePage to the cpp file as it's not used in this file, I did the same with the ARM sim.

Also a style nit: indent public with 2 spaces and { goes on its own line.

@@ +122,5 @@
> +    char *cachedData(int offset) {
> +        return &data_[offset];
> +    }
> +
> +private:

Style nit: indent with 2 spaces.

@@ +134,5 @@
> +
> +class Simulator {
> +    friend class Redirection;
> +    friend class MipsDebugger;
> +public:

Same here and a few more public: and private: below.
Attachment #8407412 - Flags: review?(jdemooij)
Assignee

Comment 7

5 years ago
(In reply to Nathan Froyd (:froydnj) from comment #5)
> Comment on attachment 8407412 [details] [diff] [review]
> Simulator-mips.patch
> 
> Review of attachment 8407412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't go over the instructions details with a fine-toothed comb, nor did
> I look deeply at the simulator interface.  Jan is better equipped to deal
> with the latter.
> 
> ::: js/src/jit/mips/Simulator-mips.cpp
> @@ +578,5 @@
> > +    js_delete(srt);
> > +}
> > +
> > +// This macro provides a platform independent use of sscanf.
> > +#define sscanF sscanf  // NOLINT
> 
> What's the rationale behind this?  Does some linter warn that you #include'd
> sscanf and you're not using it?
> 

This is taken from V8, define will be removed, sscanf will be used instead of sscanF.

> @@ +1494,5 @@
> > +{
> > +    MOZ_ASSERT((fpureg >= 0) && (fpureg < Simulator::FPURegister::kNumFPURegisters)
> > +           && ((fpureg % 2) == 0));
> > +    return *mozilla::BitwiseCast<int64_t*>(
> > +               const_cast<int32_t*>(&FPUregisters_[fpureg]));
> 
> I'd argue that BitwiseCast should be reinterpret_cast here.
> 
> But what I'd really worry about here and the functions below (and their set*
> counterparts) is endianness.  I guess you're assuming that your simulated
> MIPS always has host endianness?  (I think we discussed this before, but I
> can't remember what the verdict was.)
> 

There is an assert in the build time, checking the host. For now, we will support only x86 host.

> @@ +1674,5 @@
> > +    }
> > +    printf("Unaligned (double) read at 0x%08x, pc=0x%08x\n",
> > +           addr,
> > +           reinterpret_cast<intptr_t>(instr));
> > +    MOZ_CRASH();
> 
> Why the inconsistency in error behavior between unaligned reads/writes of
> non-words and unaligned reads/writes of words?  I'd think you'd want to drop
> into the debugger all the time.
> 

Generally, I think that we should have two different modes, test and debug. In testing mode we should just call MOZ_CRASH(), in debugging mode we can drop into the debugger.
I will use MOZ_CRASH() for now. For the mode switch, we can create another bug. What do you think?

> @@ +1854,5 @@
> > +
> > +        int32_t *stack_pointer = reinterpret_cast<int32_t*>(getRegister(sp));
> > +        // Args 4 and 5 are on the stack after the reserved space for args 0..3.
> > +        int32_t arg4 = stack_pointer[4];
> > +        int32_t arg5 = stack_pointer[5];
> 
> This is all ABI-specific, right?  You should really guard such things with
> preprocessor checks.
> 

Yes, you are right. Currently, we are supporting only O32 ABI, and I will guard this with preprocessors check, N32 ABI will be supported soon.
Assignee

Comment 8

5 years ago
Posted patch Simulator-mips.patch (obsolete) — Splinter Review
Attachment #8407412 - Attachment is obsolete: true
Attachment #8412481 - Flags: review?(nfroyd)
Attachment #8412481 - Flags: review?(jdemooij)
Assignee

Comment 9

5 years ago
Attachment #8412577 - Flags: review?(gerv)
Comment on attachment 8412577 [details] [diff] [review]
V8-licence.patch

r=gerv.

Gerv
Attachment #8412577 - Flags: review?(gerv) → review+
Comment on attachment 8412481 [details] [diff] [review]
Simulator-mips.patch

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

r=me, apologies for not catching these comments the first time through.

::: js/src/jit/mips/Simulator-mips.cpp
@@ +530,5 @@
> +    }
> +    ICacheMap &icache() {
> +#ifdef JS_THREADSAFE
> +        MOZ_ASSERT(lockOwner_ == PR_GetCurrentThread());
> +#endif

I might pull this out into:

void AssertThreadOwnership() {
#ifdef JS_THREADSAFE
    MOZ_ASSERT(...);
#endif
}

and then call it unconditionally from all the places it makes sense.  Your choice.

@@ +2558,5 @@
> +              case ff_cvt_l_fmt: {  // Mips32r2: Truncate float to 64-bit long-word.
> +                float rounded = truncf(fs_value);
> +                i64 = static_cast<int64_t>(rounded);
> +                setFpuRegister(fd_reg, i64 & 0xffffffff);
> +                setFpuRegister(fd_reg + 1, i64 >> 32);

Maybe you want a setFpuRegisterLong for encapsulating these calls, here and below.  (The double cases, too.)

@@ +3350,5 @@
> +    int32_t s6_val = getRegister(s6);
> +    int32_t s7_val = getRegister(s7);
> +    int32_t gp_val = getRegister(gp);
> +    int32_t sp_val = getRegister(sp);
> +    int32_t fp_val = getRegister(fp);

Completely optional, but something like:

const Register calleeSavedRegisters[] = { s0, s1, ... };
int32_t oldValues[MOZ_ARRAY_LENGTH(calleeSavedRegisters)];

for (size_t i = 0; i < MOZ_ARRAY_LENGTH(oldValues); ++i)
    oldValues[i] = getRegister(calleeSavedRegisters[i]);

...and so on for the blocks of code in this function would be much nicer.

::: js/src/jit/mips/Simulator-mips.h
@@ +332,5 @@
> +    static const uint32_t kNumOfWatchedStops = 256;
> +
> +
> +    // Stop is disabled if bit 31 is set.
> +    static const uint32_t kStopDisabledBit = 1 << 31;

Nit: this wants to be 1U << 31 so you don't invoke undefined behavior.
Attachment #8412481 - Flags: review?(nfroyd) → review+
Attachment #8412481 - Flags: review?(jdemooij) → review+
Assignee

Comment 12

5 years ago
Posted patch Simulator-mips.patch (obsolete) — Splinter Review
Carry reviews from the previous patch.
Attachment #8412481 - Attachment is obsolete: true
Attachment #8413814 - Flags: review+
Assignee

Comment 13

5 years ago
Assignee

Comment 14

5 years ago
Attachment #8415301 - Attachment is obsolete: true
Attachment #8418026 - Flags: review?(jdemooij)
Comment on attachment 8418026 [details] [diff] [review]
Simulator-mips-common-code.patch

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

This looks pretty straight-forward, good.

If we ever add a 3rd simulator it'd be nice to have a single define (JS_JIT_SIMULATOR or something) so that we don't have to check all 3 defines separately, but this is fine for now.
Attachment #8418026 - Flags: review?(jdemooij) → review+
Assignee

Comment 16

5 years ago
Carry reviews from the previous patch.
Attachment #8407523 - Attachment is obsolete: true
Attachment #8420117 - Flags: review+
Assignee

Comment 17

5 years ago
Carry reviews from the previous patch.
Attachment #8413814 - Attachment is obsolete: true
Attachment #8420118 - Flags: review+
Assignee

Comment 18

5 years ago
Attachment #8420119 - Flags: review?(jdemooij)
Attachment #8420119 - Flags: review?(jdemooij) → review+
Reporter

Comment 19

5 years ago
Need to check-in these patches. 

Note: Make sure that Simulator-mips-follow-up-changes.patch is applied last. 

The try link is here:
https://tbpl.mozilla.org/?tree=Try&rev=c54a0710c45b
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.