Closed
Bug 1437039
Opened 6 years ago
Closed 6 years ago
[MIPS32] - Simplify 64-bit branching
Categories
(Core :: JavaScript Engine: JIT, enhancement, P5)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: milan.knezevic, Assigned: milan.knezevic)
References
Details
Attachments
(1 file, 3 obsolete files)
25.39 KB,
patch
|
milan.knezevic
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•6 years ago
|
||
The branch64 method's logic is simplified and moved to cmp64Set.
Assignee | ||
Updated•6 years ago
|
Component: General → JavaScript Engine: JIT
Comment 2•6 years ago
|
||
Milan, is the patch ready for review yet?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(milan.knezevic)
Priority: -- → P5
Assignee | ||
Comment 3•6 years ago
|
||
Yes, patch is ready for review now.
Attachment #8949740 -
Attachment is obsolete: true
Flags: needinfo?(milan.knezevic)
Comment 4•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8950070 -
Flags: review?(lhansen)
Comment 5•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8950070 -
Flags: review?(dragan.mladjenovic)
Comment 6•6 years ago
|
||
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+
Comment 7•6 years ago
|
||
Oh, and btw, that probably needs to be JS_CODEGEN_MIPS32 does it not?
Assignee | ||
Comment 8•6 years ago
|
||
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+
Comment 9•6 years ago
|
||
Yeah, looks nice. Can this land now or is it blocked on other code landing first?
Assignee | ||
Comment 10•6 years ago
|
||
The wasm baseline code should be landed before this code.
Updated•6 years ago
|
Assignee: nobody → milan.knezevic
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8950896 -
Attachment is obsolete: true
Attachment #8952700 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 12•6 years ago
|
||
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb41cefd7f70 [MIPS32] - Simplify 64-bit branching. r=lth
Keywords: checkin-needed
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb41cefd7f70
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•