Closed Bug 1218644 Opened 6 years ago Closed 6 years ago

OdinMonkey: MIPS64: Add support for Loongson3

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file)

The Loongson CPU-s use 16k page size, this bug enable asm.js on it.
The macro '_MIPS_ARCH_LOONGSON3A' is enabled with '-march=loongson3a' of GCC.
Attachment #8679218 - Flags: review?(arai.unmht)
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.)
(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.
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 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.
Attachment #8679218 - Flags: review?(arai.unmht)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1218643
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attachment #8679218 - Flags: review?(lhansen)
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.
Attachment #8679218 - Flags: review?(lhansen) → review+
https://hg.mozilla.org/mozilla-central/rev/cd06102276f5
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.