Closed
Bug 1010334
Opened 10 years ago
Closed 10 years ago
IonMonkey: Implement Rsh Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bbouvier, Assigned: inanc.seylan)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][mentor=bbouvier][lang=c++])
Attachments
(1 file, 2 obsolete files)
5.23 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
Implement RRsh in js/src/jit/Recover.cpp. See Bug 1003801 comment 0 for explanation.
Assignee | ||
Comment 1•10 years ago
|
||
I want to work on this; hope it's alright.
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Inanc Seylan from comment #1) > I want to work on this; hope it's alright. Sure! Feel free to write a patch and post it here. If you have some questions or need some help of any kind, don't hesitate to ask here, or on IRC in our #jsapi channel.
Assignee | ||
Comment 3•10 years ago
|
||
The code and the two test cases: one for integers and one for objects.
Attachment #8423102 -
Flags: review?(benj)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8423102 [details] [diff] [review] rrsh.patch Review of attachment 8423102 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks! Just a single thing I would like to see addressed before r+, see below. It will need to get rebased too, as RBitOr landed earlier today. When you'll rebase it, can you please keep the order of instructions as specified in bug 1003801? When that's done, you can ask again for a review and I'll r+ it ;) ::: js/src/jit/Recover.cpp @@ +211,5 @@ > +bool > +RRsh::recover(JSContext *cx, SnapshotIterator &iter) const > +{ > + RootedValue lhs(cx, iter.read()); > + RootedValue rhs(cx, iter.read()); nit: could you MOZ_ASSERT here that lhs and rhs aren't objects, please?
Attachment #8423102 -
Flags: review?(benj) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
I've updated my local copy but still don't see the RBitOr changes. I guess I'll wait in this case.
Comment 6•10 years ago
|
||
(In reply to Inanc Seylan from comment #5) > I've updated my local copy but still don't see the RBitOr changes. I guess > I'll wait in this case. My guess: you're probably pulling from mozilla-central. The patch for RBitOr landed on mozilla-inbound and will be merged to central at some point by our awesome tree sheriffs. You can configure your repository to pull from inbound by changing "central" to "inbound" in your .hg/hgrc file. That means that there is a slight chance of pulling broken changesets, but I haven't personally experienced that in the ~15 months I've been using inbound as my upstream repository.
Comment 7•10 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #6) > You can configure your repository to pull from > inbound by changing "central" to "inbound" in your .hg/hgrc file. Specifically, replace "mozilla-central" with "integration/mozilla-inbound" ;)
Assignee | ||
Comment 8•10 years ago
|
||
>
> Specifically, replace "mozilla-central" with "integration/mozilla-inbound" ;)
Awesome, thanks!
Assignee | ||
Updated•10 years ago
|
Attachment #8423102 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8423249 -
Flags: review?(benj)
Assignee | ||
Comment 10•10 years ago
|
||
> It will need to get rebased too, as RBitOr landed earlier today. When you'll > rebase it, can you please keep the order of instructions as specified in bug > 1003801? done > > nit: could you MOZ_ASSERT here that lhs and rhs aren't objects, please? done Thank you for your help!
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8423249 [details] [diff] [review] rrsh.patch Review of attachment 8423249 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks for your patch! If you can, please set the checkin-needed keyword, so that a sheriff land it. Otherwise, I'll do it on your behalf. Thanks again!
Attachment #8423249 -
Flags: review?(benj) → review+
Assignee | ||
Comment 12•10 years ago
|
||
> If you can, please set the checkin-needed keyword, so that a sheriff land
> it. Otherwise, I'll do it on your behalf. Thanks again!
Sorry, I can not set keywords. Thank you!
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Inanc Seylan from comment #12) > > If you can, please set the checkin-needed keyword, so that a sheriff land > > it. Otherwise, I'll do it on your behalf. Thanks again! > > Sorry, I can not set keywords. Thank you! No worries, I did it for you, thanks again! Keep up the good work,there are still other bugs this kind on which bug 1003801 depends, or if you want some other kind of bug, don't hesitate to ask here or on IRC #jsapi ;)
Keywords: checkin-needed
Comment 14•10 years ago
|
||
Per discussion with Ben on IRC, he's going to run this through Try before setting checkin-needed.
Keywords: checkin-needed
Reporter | ||
Comment 15•10 years ago
|
||
Comment on attachment 8423249 [details] [diff] [review] rrsh.patch Review of attachment 8423249 [details] [diff] [review]: ----------------------------------------------------------------- Could you please rebase your patch on inbound, so that it applies cleanly? (hint: don't forget to order your methods / class / macro names as indicated in the meta bug ;)) ::: js/src/jit/Recover.h @@ +19,5 @@ > #define RECOVER_OPCODE_LIST(_) \ > _(ResumePoint) \ > _(BitNot) \ > _(BitOr) \ > + _(Rsh) \ nit: can you align with the other backslashes on the right please?
Assignee | ||
Updated•10 years ago
|
Attachment #8423249 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8428582 -
Flags: review?(benj)
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #15) > > Could you please rebase your patch on inbound, so that it applies cleanly? > (hint: don't forget to order your methods / class / macro names as indicated > in the meta bug ;)) Done > > ::: js/src/jit/Recover.h > @@ +19,5 @@ > > #define RECOVER_OPCODE_LIST(_) \ > > _(ResumePoint) \ > > _(BitNot) \ > > _(BitOr) \ > > + _(Rsh) \ > > nit: can you align with the other backslashes on the right please? Done
Reporter | ||
Comment 18•10 years ago
|
||
Comment on attachment 8428582 [details] [diff] [review] rrsh.patch Review of attachment 8428582 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks!
Attachment #8428582 -
Flags: review?(benj) → review+
Reporter | ||
Comment 19•10 years ago
|
||
I've pushed it to try on your behalf. For making it easier to contribute, if you want, you can ask for Try server access, which will give you the permission to make test pushes on our continuous integration system and will help have your patches landed quicker. If you're interested in having such rights, follow the procedure in [1] (try push access is Level 1) and CC nbp and me on the bug, so that we can vouch for you. Thanks once more for your contributions! [1] https://www.mozilla.org/hacking/committer/ https://tbpl.mozilla.org/?tree=Try&rev=8dc509c95e01
Reporter | ||
Comment 20•10 years ago
|
||
Looks good to me! Can a sheriff please land it once the tree is re-opened? Thanks!
Keywords: checkin-needed
Reporter | ||
Comment 21•10 years ago
|
||
Rebased it for you, as RLsh landed in the meanwhile ;) https://hg.mozilla.org/integration/mozilla-inbound/rev/98fb71452e0c
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98fb71452e0c
Assignee: nobody → inanc.seylan
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•