Closed Bug 1437039 Opened 2 years ago Closed 2 years ago

[MIPS32] - Simplify 64-bit branching

Categories

(Core :: JavaScript Engine: JIT, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: milan.knezevic, Assigned: milan.knezevic)

References

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.119 Safari/537.36 OPR/51.0.2830.26
Attached patch 64bit-branches.diff (obsolete) — Splinter Review
The branch64 method's logic is simplified and moved to cmp64Set.
Depends on: wasm-mips
Component: General → JavaScript Engine: JIT
Milan, is the patch ready for review yet?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(milan.knezevic)
Priority: -- → P5
Attached patch 64bit-branches.diff (obsolete) — Splinter Review
Yes, patch is ready for review now.
Attachment #8949740 - Attachment is obsolete: true
Flags: needinfo?(milan.knezevic)
Comment on attachment 8950070 [details] [diff] [review]
64bit-branches.diff

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

Dragan, would you be the right person to review these patches?
Attachment #8950070 - Flags: review?(dragan.mladjenovic)
Attachment #8950070 - Flags: review?(lhansen)
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8950070 [details] [diff] [review]
> 64bit-branches.diff
> 
> Review of attachment 8950070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Dragan, would you be the right person to review these patches?

I rather have a third party to review this first.
Attachment #8950070 - Flags: review?(dragan.mladjenovic)
Comment on attachment 8950070 [details] [diff] [review]
64bit-branches.diff

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

Ship it!  This is obviously the best fix also for bug 1316822, so once you've landed this I'll probably just promote cmp64Set to MacroAssembler.h and implement it properly everywhere.

::: js/src/jit-test/tests/wasm/integer.js
@@ +301,5 @@
>  testComparison64('gt_u', 40, 40, 0);
>  testComparison64('ge_s', 40, 40, 1);
>  testComparison64('ge_u', 40, 40, 1);
>  testComparison64('eq', "0x400012345678", "0x400012345678", 1);
> +testComparison64('eq', "0x400012345678", "0x500012345678", 0);

Thank you for adding more test cases!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +3824,5 @@
>      void cmp64Set(Assembler::Condition cond, RegI64 lhs, RegI64 rhs, RegI32 dest) {
>  #ifdef JS_PUNBOX64
>          masm.cmpPtrSet(cond, lhs.reg, rhs.reg, dest);
>  #else
> +# if defined(JS_MIPS32)

Can you change the ifdef above to if defined, and change this to an elif?
Attachment #8950070 - Flags: review?(lhansen) → review+
Oh, and btw, that probably needs to be JS_CODEGEN_MIPS32 does it not?
Attached patch 64bit-branches_14022018.diff (obsolete) — Splinter Review
Thank you for review, and sorry for my sloppy changes. The patch is changed according to your comments.
Attachment #8950070 - Attachment is obsolete: true
Attachment #8950896 - Flags: review+
Yeah, looks nice.  Can this land now or is it blocked on other code landing first?
The wasm baseline code should be landed before this code.
Assignee: nobody → milan.knezevic
Attachment #8950896 - Attachment is obsolete: true
Attachment #8952700 - Flags: review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb41cefd7f70
[MIPS32] - Simplify 64-bit branching. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb41cefd7f70
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.