Closed
Bug 1008110
Opened 10 years ago
Closed 10 years ago
IonMonkey: Implement BitAnd Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: nbp, Assigned: inanc.seylan)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][mentor=nbp][lang=c++])
Attachments
(2 files, 2 obsolete files)
5.79 KB,
patch
|
Details | Diff | Splinter Review | |
5.34 KB,
patch
|
nbp
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
Implement RBitAnd in js/src/jit/Recover.cpp. See Bug 1003801 comment 0 for explanations.
Assignee | ||
Comment 1•10 years ago
|
||
Hi, Here's an implementation of the RBitNot recover instruction. I've tested it in a separate .js file that is not in the repository. The test logic is as in dce-with-rinstructions.js. It recovers well, i.e., the assertion is valid and I see the gray text in the generated iongraph output.
Assignee | ||
Comment 2•10 years ago
|
||
Sorry, this patch is supposed to be for the bug https://bugzilla.mozilla.org/show_bug.cgi?id=1003802
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8420624 [details] [diff] [review] rbitnot.patch Review of attachment 8420624 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good :) Add a test case for it and we would integrate it in Firefox ;) ::: js/src/jit/Recover.cpp @@ +208,5 @@ > + > +RBitNot::RBitNot(CompactBufferReader &reader) > +{ > + > +} style-nit: remove this empty line. ::: js/src/jit/Recover.h @@ +19,5 @@ > #define RECOVER_OPCODE_LIST(_) \ > _(ResumePoint) \ > _(Add) \ > + _(NewDerivedTypedObject) \ > + _(BitNot) nit: I suggest we add it just before the Add and after the ResumePoint, to group them by categories, as done in Bug 1003801
Attachment #8420624 -
Flags: feedback+
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8420624 [details] [diff] [review] rbitnot.patch Check the bug https://bugzilla.mozilla.org/show_bug.cgi?id=1003802 instead.
Attachment #8420624 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
Attachment #8424446 -
Flags: review?(nicolas.b.pierron)
Comment 6•10 years ago
|
||
Attachment #8424446 -
Attachment is obsolete: true
Attachment #8424446 -
Flags: review?(nicolas.b.pierron)
Attachment #8424449 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8424449 [details] [diff] [review] patchv1 Review of attachment 8424449 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js @@ +114,5 @@ > return i; > } > > +var uceFault_and_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_and_object')); > +function rbitand_object(i) { Group these functions, and follow the same order as listed in Bug 1003801 comment 0. ::: js/src/jit/Recover.h @@ +22,1 @@ > _(BitNot) \ Follow the order, as listed in Bug 1003801 comment 0.
Attachment #8424449 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8428608 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 9•10 years ago
|
||
Fixed the small problems in Sankha's patch and asserted that lhs and rhs are not objects in RBitAnd::recover()
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8428608 [details] [diff] [review] rbitand.patch Review of attachment 8428608 [details] [diff] [review]: ----------------------------------------------------------------- This patch sounds good, can you send it to try to check[1] JS failures, such as rooting issues, Valgrind and Asan builds, and run the jittests and the jsreftests. If you need to request Try access, you can needinfo me and/or benj. Once the all the entries are green, add the checkin-needed keyword in the bug, such as a sheriff or me can push the bug for you ;) [1] http://trychooser.pub.build.mozilla.org/
Attachment #8428608 -
Flags: review?(nicolas.b.pierron) → review+
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → inanc.seylan
Reporter | ||
Updated•10 years ago
|
Attachment #8428608 -
Flags: checkin?(nicolas.b.pierron)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8428608 [details] [diff] [review] rbitand.patch Review of attachment 8428608 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, I forgot to push this patch after doing the review. https://tbpl.mozilla.org/?tree=Try&rev=18b49b73f250 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0c18b3fead
Attachment #8428608 -
Flags: checkin?(nicolas.b.pierron) → checkin+
https://hg.mozilla.org/mozilla-central/rev/7b0c18b3fead
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•