1329650 - Fix compilation failure for simulator builds on MIPS32 and MIPS64.

Status: RESOLVED FIXED

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

Description

[ivica.bogosavljevic]

Attachment: out.patch

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

Steps to reproduce:

Hi!

I am interested in adding support for MIPS to SpiderMonkey. I see that the support is already there, but it hasn't been maintained for some time. The simulator builds fail to compile, and the target builds do compile, but there are a handful of tests that fail (~ 1% of tests are failing).

I plan to put some effort in order to get MIPS ports to work again.

Please find attached patch that fixes problems with simulator compilation for MIP32 and MIPS64


Actual results:

The simulator builds don't compile


Expected results:

The simulator builds should compile

Comment 1

[Nicolas B. Pierron [:nbp]]

Welcome, and thanks for your interest.

I will forward this issue to few persons who might know better about our MIPS backend.
Otherwise, I might be able to do the review in roughly a month.

Comment 2

[ivica.bogosavljevic]

I think Branislav Rankov is not longer involved in MIPS port.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1329650-mips32.patch

Make MIPS32 compile and link, and fix a bunch of warnings.

This is not yet correct or complete but it's somewhere in the neighborhood.  The register allocation code is copied from ARM and is not closely inspected.  storeRegistersInMask is known not to be correct for FP regs (MOZ_CRASH).

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1329650-mips64.patch

Make Mips64 compile and link, and fix some warnings.

This crashes on every test in baseline-generated code (the SP has a garbage value).  I have not had the time to dig deeply.

Comment 5

[Lars T Hansen [:lth]]

Specifically these patches both fail "dist/bin/jsapi-tests Register" so that's a place to start.  (I'll come back to that in a day or two if nobody beats me to it.)

Comment 6

[ivica.bogosavljevic]

Hi Lars!

I had a look at your patch and I see that you introduced some new things in MIPS ports. I don't understand however, how the patches you provided fit  together with the patches I have provided. Are they going to be committed together? Do I need to provide some changes as well, or you will do them?

I also fixed some other bugs (mostly simple bugs in the simulator) and plan to send the patches as new issues in bugzilla. The idea is to slowly fix as much as issues as possible going from simpler to more complex. 

After simulator is fixed we will fix remaining issues on the board.

Comment 7

[Lars T Hansen [:lth]]

Firstly, the patches I put here incorporate (or are certainly intended to incorporate) the fixes you made, so your patch should now be obsolete.  I could have marked as such but since I'm not asking for reviews right now I thought it was not necessary yet.

Secondly, there are some bugs in the patches I uploaded in the management of register sets (a new Jit API that landed recently).  I have fixed the bugs for MIPS64 but not yet for MIPS32.  When I have fixed MIPS32 I will upload new patches for both.  That is not to say that we want to land those patches but at least things will be compiling and linking and passing some selftests.

I could actually use some advice on the MIPS32 code:  In Architecture-mips32.h there is a comment block that explains that MIPS32 can have two types of FP coprocessors, one that effectively looks like ARM (16 double-precision registers that alias 32 single-precision registers arranged as pairs) and one that effectively looks like ARM64 (32 double-precision registers that can also be used as single-precision).  So far so good.  But can we know at compile time which FPU configuration we're compiling for?  And if so, how do we do that?   The reason I ask is that the register sets need to be implemented differently for the two cases, unless I reduce it to a common configuration (which would be 16 double-precision registers that each aliases one single-precision one, ie, a configuration that looks like x86).  I can do that, and it might be OK to do so if it gets the code running again, but if you have opinions I'd love to hear them.

Comment 8

[ivica.bogosavljevic]

Hi Lars!

Thank you for your involvement on this. MIPS32 CPU does indeed support wto modes FP32 (16 double registers aliased with 32 single registers) and FP64 (32 double registers)  like you described.

For the simulator builds, we cannot deduce this from the compilation flags. Inspection of the simulator reveals that behavior is FP32 -> LDC1, SDC1 etc. load the upper part of double to the odd register.

For the target builds, compilation flags -mfp32 and -mfp64 are used to specify FP32 or FP64 mode. Dynamic linker sets the appropriate flags in the CPU so the binary can execute correctly. I am not completely sure, but I guess we can get these information from the compiler.

For the target builds, we can get these information at runtime by executing a short sequence of code that clears e,g. f0 and f1, writes a double to f0, and then checks if f1 has changed. v8 uses this method: https://github.com/v8/v8/blob/master/src/base/cpu.cc#L140

However, unless it is advantageous to have 32 floating point registers instead of 16 (on v8 it isn't), we could reduce FP32 and FP64 to a single mode. We achieve this by emitting code that is FPXX, meaning that the same code can execute in FP32 and FP64 code without difference. FPXX needs to have some limitations though:
* We are limited to even registers only, both for single precision and double precision values. We cannot do any operations that rely on the fact that higher part of the double value is in odd register.

So these are scenarios we need to cover:
* Loading and storing double from and to memory: use LDC1 and SDC1
* Loading and storing double from and to general purpose registers: this is the tricky case.
 * On MIPS32R2 and newer, you would use MTHC1 and MFHC1 (move from/to high coprocessor 1) instruction, e.g. 
    MTC1 f0, a0
    MTHC1 f0, a1
 * On MIPS32R1 and older, we would need to use a specialized memory location for this:
    SW a0, 0[t8]
    SW a1, 4[t8]
    LDC1 f0, 0[t8]
* Other instructions know what registers to use depending on the FP mode.

MIPS32R2 is the actual version of the architecture and supports both FP32 and FP64, MIPS32R1 is used a lot but I don't think any new designs will be done on it. It supports FP32. MIPS32R6 is the future architecture which will support FP64 only!

Comment 9

[ivica.bogosavljevic]

I just noticed that the HEAD doesn't compile anymore even with your patch because of newly introduced

MacroAssembler::call(const Address& addr)

and errors because of the rename of JitRuntime to JitZoneGroup.

Should I create new patches based on your patches or let you fix this?

Comment 10

[Lars T Hansen [:lth]]

I'm sorry for not keeping up with my previous patch but I have a lot to do at the moment :-/ and MIPS is not really a priority.  Anything you can do to keep up with changes is good.  That includes small fixes like what you're mentioning in comment 9 as well as trying to finish my bigger patch for the register sets.

The JIT code is changing very rapidly right now and for MIPS there are two options: either keep up daily (!) with changes to avoid bitrot, or wait until the rate of change is lower, maybe around the time we branch in early March.  No guarantees that it will get slower, though...

Comment 11

[ivica.bogosavljevic]

Attachment: bug1329650-mips3264-2.patch

Hi Lars!

I rebased your patch to master head, so it compiles again. I removed ICacheCheckingEnabled in the simulator because it is just set and not used anywhere.


Base commit:
commit a32e53440ca08146300593584ac9b2913adfb5f2
Merge: 16d6714 38a1059
Author: Phil Ringnalda <[e-mail]>
Date:   Mon Feb 20 20:28:42 2017 -0800

    Merge m-i to m-c, a=merge

Comment 12

[Lars T Hansen [:lth]]

(In reply to ivica.bogosavljevic from comment #11)
> Created attachment 8839436 [details] [diff] [review]
> bug1329650-mips3264-2.patch
> 
> I rebased your patch to master head, so it compiles again. I removed
> ICacheCheckingEnabled in the simulator because it is just set and not used
> anywhere.

The problem is that I know for a fact that my code for register sets on mips32 is wrong, see our discussion in comment 7, comment 8, and I don't think we should land that code if we know it is wrong.  Did you fix that?

If not, then if we could perhaps restrict this patch to the mips64 port initially then I would be happy to review it so that we could at least reduce the scope of the problem.  We could then work on getting the mips32 code fixed separately.

(I'll investigate ICacheCheckingEnabled, this may be an ARM thing and it may be OK to leave it out.)

Comment 13

[Lars T Hansen [:lth]]

Comment on attachment 8839436 [details] [diff] [review]
bug1329650-mips3264-2.patch

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

(Clearing r? for the time being)

Comment 14

[ivica.bogosavljevic]

(In reply to Lars T Hansen [:lth] from comment #12)
> (In reply to ivica.bogosavljevic from comment #11)
> > Created attachment 8839436 [details] [diff] [review]
> > bug1329650-mips3264-2.patch
> > 
> > I rebased your patch to master head, so it compiles again. I removed
> > ICacheCheckingEnabled in the simulator because it is just set and not used
> > anywhere.
> 
> The problem is that I know for a fact that my code for register sets on
> mips32 is wrong, see our discussion in comment 7, comment 8, and I don't
> think we should land that code if we know it is wrong.  Did you fix that?
> 
> If not, then if we could perhaps restrict this patch to the mips64 port
> initially then I would be happy to review it so that we could at least
> reduce the scope of the problem.  We could then work on getting the mips32
> code fixed separately.
> 
> (I'll investigate ICacheCheckingEnabled, this may be an ARM thing and it may
> be OK to leave it out.)

Hi Lars!

I am sorry for late reply. 
In FP32: f0, f1, f2 .. f31 are 32 bit wide float registers. If we want to use 64 bit registers, we have f0, f2, ... f30 at our disposal, but everytime we write to double f0, both float f0 and float f1 are modified.
In FP64: f0, f1 .. f31 are 64 bit wide registers. We can use to store float, in that case upper 32bits are not modified.

We can reduce this to common case: disable allocation of odd registers. This is how it is done on v8 JIT compiler. With a few modifications, we can emit MIPS instructions that execute with the same semantics in FP32 and FP64 environment.

Comment 15

[Miran Karic]

Attachment: bug1329650-mips32-3.patch

Hello!

I've been working on this issue and here I upload a patch that fixes mips32 simulator build with the latest version of code. Some problems you mention earlier still remain but I will be working on fixing this as well. Any comments are welcome.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

See also bug 1389401 which contains a few patches to fix MIPS build.

Comment 17

[Dragan Mladjenovic]

Attachment: Rebase of bug1329650-mips32-3.patch onto changes done in bug 1389401.

Hello!

I've managed to cleanup the Miran's patch excluding changes done in bug 1389401. I've also added handling for a few more instruction into mips32 simulator. The handling of FP register set is left the same as in previous patches. I would like to tackle this issue implementing FPXX-like handling of FP regiters on mips32. Do I need to open another issue or can I reuse existing one?
Please take a look.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Hi! Are you saying this patch is needed on top of the patches in bug 1389401? If so, we can keep this particular patch here in this particular bug; if you need a review for this patch, please set the review? flag with a name next to it. Other work that is not related to the build failure should be done in another bug. Thanks!

Comment 19

[Dragan Mladjenovic]

Attachment: Rebase of bug1329650-mips32-3.patch onto changes done in bug 1389401.

Fixed incorrectly rebased hunk from previous patch.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8896981 [details] [diff] [review]
Rebase of bug1329650-mips32-3.patch onto changes done in bug 1389401.

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

Thanks for doing this! There's some impressive amount of work that should fix at least builds and probably a few defects.

I'd like to see these changes one more time after the comments below have been addressed. Ideally, it would be split in a few smaller ones: for instance, the build fixes should go to one separated patch, the new instructions impl should go in another patch. The new instructions patch should be flagged for review by :lth (in addition to or without me). I'm also fine with having functional fixes done later, in a follow-up bug.

Please make sure to also include a commit message, as indicated in https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F (it'd be nice to have more context too, 8 lines is the recommended value, as far as I recall)

::: js/src/jit/mips32/Architecture-mips32.h
@@ +317,5 @@
> +
> +template <> inline FloatRegister::SetType
> +FloatRegister::AllocatableAsIndexableSet<RegTypeName::Float64>(SetType set)
> +{
> +    // (Copied from ARM)

Can you confirm my understanding: MIPS32 only has 32 bits FP registers, each double instruction operates on 2 32-bits FP registers, so we can basically apply the same aliasing rules as on ARM. Is that correct?

::: js/src/jit/mips32/Simulator-mips32.cpp
@@ +38,5 @@
>  #include "jit/mips32/Assembler-mips32.h"
>  #include "vm/Runtime.h"
> +#include "wasm/WasmInstance.h"
> +#include "wasm/WasmSignalHandlers.h"
> +#include "jit/AtomicOperations.h"

nit: includes must be ordered in alphabetical order.

In your build directory, please make sure `make check-style` passes. It will also help you ordering correctly all the includes.

@@ +1266,4 @@
>      pc_modified_ = false;
>      icount_ = 0;
>      break_count_ = 0;
> +    wasm_interrupt_ = false;

Can you also initialize all the LL fields?

@@ -1629,5 @@
> -// simply disallow unaligned reads, but at some point we may want to move to
> -// emulating the rotate behaviour.  Note that simulator runs have the runtime
> -// system running directly on the host system and only generated code is
> -// executed in the simulator.  Since the host is typically IA32 we will not
> -// get the correct MIPS-like behaviour on unaligned accesses.

Why is this comment removed? I think nothing properly handles the unaligned reads and writes yet in this patch?

@@ +1648,5 @@
> +void
> +Simulator::handleWasmInterrupt()
> +{
> +    void* pc = (void*)get_pc();
> +    void* fp = (void*)getRegister(s8);

Why did you choose s8 here? It seems FramePointer is defined in mips-shared/Assembler-mips-shared.h to an InvalidPointer, and the GenerateCallablePrologue in wasm/WasmFrameIter.cpp doesn't seem prepared to handle MIPS at all? (happy to defer any function changes to another bug, if you'd prefer, though)

@@ +1664,5 @@
> +    startInterrupt(activation);
> +    set_pc(int32_t(segment->interruptCode()));
> +}
> +
> +

nit: please remove one blank line

@@ +1680,5 @@
> +    if (!act)
> +        return false;
> +
> +    void* pc = reinterpret_cast<void*>(get_pc());
> +    uint8_t* fp = reinterpret_cast<uint8_t*>(getRegister(s8));

same comment as above for the choice of fp

@@ +1685,5 @@
> +    wasm::Instance* instance = wasm::LookupFaultingInstance(act, pc, fp);
> +    if (!instance || !instance->memoryAccessInGuardRegion((uint8_t*)addr, numBytes))
> +        return false;
> +
> +    llBit_ = false;

Could you detail in comments the reasons to unset the exclusive access monitor in any case, please?

@@ -1635,5 @@
>  int
>  Simulator::readW(uint32_t addr, SimInstruction* instr)
>  {
> -    if (addr < 0x400) {
> -        // This has to be a NULL-dereference, drop into debugger.

Can you explain why this was removed?

@@ +1724,5 @@
>  void
>  Simulator::writeW(uint32_t addr, int value, SimInstruction* instr)
>  {
> +    if (handleWasmFault(addr,4)) {
> +      return;

nit: we use 4-spaces indents. There are a few other instances in this code.

nit: no braces around single-line if bodies

@@ +1743,5 @@
>  double
>  Simulator::readD(uint32_t addr, SimInstruction* instr)
>  {
> +    if (handleWasmFault(addr,8)) {
> +      return NAN;

(same nits as the above if apply here)

I am also pretty sure NAN is never explicitly defined and shouldn't be used.

@@ +1902,5 @@
> +{
> +    if ((addr & kPointerAlignmentMask) == 0) {
> +        volatile int32_t* ptr = reinterpret_cast<volatile int32_t*>(addr);
> +        int32_t value = *ptr;
> +        lastLLValue_ = value;

As said in other comment: see implementation of ARM's equivalent in readExW. That being said, I think your implementation is *more* correct, in that it also stores the address... We might need Lars's help on this, but we can get back to it later.

@@ +1927,5 @@
> +    if ((addr & kPointerAlignmentMask) == 0) {
> +        SharedMem<int32_t*> ptr = SharedMem<int32_t*>::shared(reinterpret_cast<int32_t*>(addr));
> +
> +        if (!llBit_) {
> +          return 0;

nit: 4spaces indent

@@ +1934,5 @@
> +        llBit_ = false;
> +        lastLLAddress_ = 0;
> +        int32_t expected = lastLLValue_;
> +        int32_t old = AtomicOperations::compareExchangeSeqCst(ptr, expected, int32_t(value));
> +        return (old == expected) ? 1:0;

nit: spaces between : operator

@@ +2690,4 @@
>              break;
>            case rs_cfc1:
>              setRegister(rt_reg, alu_out);
> +            MOZ_FALLTHROUGH;

Don't you want to break here?

@@ +2776,1 @@
>                  // In rounding mode 0 it should behave like ROUND.

Can you put the FALLTHROUGH below this comment?

@@ +2937,1 @@
>                  // In rounding mode 0 it should behave like ROUND.

ditto

::: js/src/jit/mips32/Simulator-mips32.h
@@ +191,5 @@
>      template <typename T>
>      T get_pc_as() const { return reinterpret_cast<T>(get_pc()); }
>  
> +    void trigger_wasm_interrupt() {
> +        MOZ_ASSERT(!wasm_interrupt_);

nit: remove the assertion and please copy ARM32 equivalent method's comment

@@ +194,5 @@
> +    void trigger_wasm_interrupt() {
> +        MOZ_ASSERT(!wasm_interrupt_);
> +        wasm_interrupt_ = true;
> +     }
> +

nit: spurious blank line

@@ +344,5 @@
>      uint32_t FCSR_;
>  
> +    bool llBit_;
> +    int32_t lastLLValue_;
> +    uint32_t lastLLAddress_;

I think these could be better named. Also I am not too sure about the implementations of loadLink/storeConditional. Can you have a look at ARM's readExW/writeExW and reuse the same naming scheme, at least, if not the same implementation?

::: js/src/shell/js.cpp
@@ +8053,4 @@
>          jit::Simulator::StopSimAt = stopAt;
>  #elif defined(JS_SIMULATOR_MIPS32) || defined(JS_SIMULATOR_MIPS64)
>      if (op.getBoolOption("mips-sim-icache-checks"))
> +        jit::SimulatorProcess::ICacheCheckingDisableCount = 0;

Can you also remove ICacheCheckingEnabled from mips32/Simulator-mips32.h?

::: js/src/wasm/WasmStubs.cpp
@@ +1138,2 @@
>      static_assert(!SupportsSimd, "high lanes of SIMD registers need to be saved too.");
>      // save all registers,except sp. After this stack is alligned.

pre-existing: can you replace this comment by the one you've deleted above, please?

@@ +1159,4 @@
>      masm.addToStackPtr(Imm32(4 * sizeof(intptr_t)));
>  # endif
>  
> +    masm.branchIfFalseBool(ReturnReg, throwLabel);

This is incorrect: HandleExecutionInterrupt returns resumePC (which is a void*) or nullptr, so the previous check was actually correct.

We have tests for this and wasm in general, that you can trigger with the following line:

js/src/jit-test/jit_test.py /path/to/built/executable/js wasm

@@ +1170,3 @@
>      masm.PopRegsInMask(AllRegsExceptSP);
>  
> +

nit: spurious blank line

Comment 21

[Dragan Mladjenovic]

Attachment: bug1329650-mips32-4.patch

Hi,
I removed all functional changes leaving only empty stubs. Functional changes will be sent as separate patches.
Can you please take a look?

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8897966 [details] [diff] [review]
bug1329650-mips32-4.patch

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

Thanks, looks good to me.

Can you add in the commit message the bug number, and r=bbouvier at the end? So it eventually looks like this:

Bug 1329650: Make MIPS32 simulator build pass; r=bbouvier

Then, you can upload the new patch and we can ask our sheriffs to check it in.

::: js/src/jit/mips32/Assembler-mips32.h
@@ +73,4 @@
>  static constexpr FloatRegister SecondScratchFloat32Reg = { FloatRegisters::f16, FloatRegister::Single };
>  static constexpr FloatRegister SecondScratchDoubleReg = { FloatRegisters::f16, FloatRegister::Double };
>  
> +

nit: please remove the spurious blank line here

Comment 23

[Dragan Mladjenovic]

Attachment: bug1329650-mips32-5.patch

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

I will mark all the other patches as obsolete (to make landing of this last one easier), since no other has landed and I assume this last patch + the patches from bug 1389401 are enough to fix the compilation failures. I haven't looked at any other patch, so there might be valuable work still there! If that's the case and you think some of these patches could be reused, let's open other bugs and leave this one alone.

Also clearing needinfos.

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

No try build; this is on MIPS only.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c217dd347b01
Make MIPS32 simulator build pass. r=bbouvier

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/c217dd347b01