1740186 - [MIPS64] Failed to build JS on mips64 platform (ma_addPtrTestCarry)

Status: RESOLVED FIXED

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

Description

[Zhao Jiazhong]

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

Steps to reproduce:

Build JS shell on mips64 platform.

Actual results:

Got error:

/mozilla-central/js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h:724:7: error: no matching function for call to 'js::jit::MacroAssembler::ma_addPtrTestCarry(js::jit::AssemblerMIPSShared::Condition&, js::jit::Register&, js::jit::Register&, js::jit::ImmWord&, js::jit::Label*&)'
        ma_addPtrTestCarry(cond, dest, dest, src, label);
              ^~~~~~~~~~~~~~~~~~
 In file included from /mozilla-central/js/src/jit/MacroAssembler.h:27,
                  from /mozilla-central/js/src/wasm/WasmGenerator.h:25,
                  from /mozilla-central/js/src/wasm/AsmJS.cpp:61,
                  from Unified_cpp_js_src_wasm0.cpp:2:

Expected results:

Build success.

Comment 1

[Zhao Jiazhong]

Attachment: Bug 1740186 - [MIPS64] Accept ImmWord operand in ma_addPtrTestCarry. r=lth

Comment 2

[Lars T Hansen [:lth]]

This patch is in principle not necessary, since the patch that landed on bug 1738997 fixed the compilation problem. I'll review the patch anyway, and you can then land it if you like, but that's up to you.

(By the way, memory64 is disabled on MIPS64, and even for memory32 we still limit the heap size to 2GB since we didn't know if the existing logic could handle values with the high bit set. Let me know if you want to fix any of that.)

Comment 3

[Zhao Jiazhong]

Thanks for your information, I found the largeBuffer is also disabled on mips64 platform due to similar reason, and I was tring to fix it, although still got some failed jit-tests to debug. So may be after my work on largeBuffer, the memory64 issue is also fixed?

And since the compilation problem is fixed now, I think this patch could wait for the memory64 patch then. Thanks for your review!

Comment 4

[Lars T Hansen [:lth]]

(In reply to Zhao Jiazhong from comment #3)

Thanks for your information, I found the largeBuffer is also disabled on mips64 platform due to similar reason, and I was tring to fix it, although still got some failed jit-tests to debug. So may be after my work on largeBuffer, the memory64 issue is also fixed?

Even if memory64 is not completely fixed by that it should be fixable without too much further work, especially since you already implemented the addPtrTestCarry code.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1736365

Comment 6

[Lars T Hansen [:lth]]

(I see this is unlanded.) If/when you need me to land a patch, needinfo me on the bug and I will take care of it.

Comment 7

[Zhao Jiazhong]

I just tested this patch along with my memory64 support patch, and it seems fine. So please land it, thanks!
And I will submit the memory64 support patch later.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29f8d97fa2d8
[MIPS64] Accept ImmWord operand in ma_addPtrTestCarry. r=lth

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/29f8d97fa2d8

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to nominate for Beta uplift for 95? Please nominate for approval if so.

Comment 11

[Zhao Jiazhong]

This patch alone doesn't make a difference, it's needed by bug 1742542, which may also rely on bug 1741165.
So we could uplift all of them after bug 1742542 is landed and bug 1741165's regression is fixed.