1218644 - OdinMonkey: MIPS64: Add support for Loongson3

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

The Loongson CPU-s use 16k page size, this bug enable asm.js on it.

Comment 1

[hev [:hev]]

Attachment: 0001-OdinMonkey-MIPS64-Add-support-for-Loongson3.patch

The macro '_MIPS_ARCH_LOONGSON3A' is enabled with '-march=loongson3a' of GCC.

Comment 2

[Lars T Hansen [:lth]]

This is going to need careful testing and probably requires more work.

In principle it should work but that code's only ever been tested with 4K pages, to my knowledge.

For example, the logic in IsValidAsmJSHeapLength() looks "wrong", ie is incompatible with this change.  It actually depends on a 4K page size, in the following way: it says that a valid heap length is always a power-of-two multiple of 4KB, so 4K is a valid heap length.  But then the code asserts that the length mod AsmJSPageSize must be zero.  If the page size is 16KB, then a 4KB page is suddenly not valid after all.  Worse, this makes valid asm.js page sizes system-dependent, which is absurd because asm.js is system-independent.  Arguably the assertion is incorrect but I don't know if it guards some deeper invariant that's used elsewhere.

RoundUpToNextValidAsmJSHeapLength() encodes the same assumptions.

For another thing, I wonder how it interacts with code in vm/ArrayBufferObject.cpp and vm/SharedArrayObject.cpp.  This code looks clean but I can't be sure - again, it's only ever been tested with 4KB pages.

(ARM64 on iOS also uses 16K pages, but the ARM64 port is roughly in the same state as the MIPS port so doesn't help us much yet.)

Comment 3

[hev [:hev]]

(In reply to Lars T Hansen [:lth] from comment #2)
> This is going to need careful testing and probably requires more work.
> 
> In principle it should work but that code's only ever been tested with 4K
> pages, to my knowledge.
> 
> For example, the logic in IsValidAsmJSHeapLength() looks "wrong", ie is
> incompatible with this change.  It actually depends on a 4K page size, in
> the following way: it says that a valid heap length is always a power-of-two
> multiple of 4KB, so 4K is a valid heap length.  But then the code asserts
> that the length mod AsmJSPageSize must be zero.  If the page size is 16KB,
> then a 4KB page is suddenly not valid after all.  Worse, this makes valid
> asm.js page sizes system-dependent, which is absurd because asm.js is
> system-independent.  Arguably the assertion is incorrect but I don't know if
> it guards some deeper invariant that's used elsewhere.
> 
> RoundUpToNextValidAsmJSHeapLength() encodes the same assumptions.
> 
> For another thing, I wonder how it interacts with code in
> vm/ArrayBufferObject.cpp and vm/SharedArrayObject.cpp.  This code looks
> clean but I can't be sure - again, it's only ever been tested with 4KB pages.
> 
> (ARM64 on iOS also uses 16K pages, but the ARM64 port is roughly in the same
> state as the MIPS port so doesn't help us much yet.)

Thank you tell me that. I never understand why odinmonkey bind to 4K page size. The octane (zlib) performance improved and jit-tests all passed after that enabled, so I think it works.

Comment 4

[Lars T Hansen [:lth]]

Any existing tests probably pass because nobody allocates a heap of just 4KB (indeed there's a warning in LinkModuleToHeap that 64KB will be required at some point in the future), but try this in a MIPS64 debug build with the page size set to 16KB:

function m(stdlib, ffi, heap) {
    "use asm";
    var h = new stdlib.Int32Array(heap);
    function f() {
        return 0;
    }
    return { f:f }
}
m(this, {}, new ArrayBuffer(4096));

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8679218 [details] [diff] [review]
0001-OdinMonkey-MIPS64-Add-support-for-Loongson3.patch

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

based on conversation, clearing r? for now.
and I think I'm not capable for reviewing this bug.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8679218 [details] [diff] [review]
0001-OdinMonkey-MIPS64-Add-support-for-Loongson3.patch

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

This should work, so let's try it until we can get the proper fix in for a runtime-determined page size.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd06102276f5

Comment 9

[Carsten Book [:Tomcat]]

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