Closed Bug 1010334 Opened 7 years ago Closed 7 years ago

IonMonkey: Implement Rsh Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

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)

Implement RRsh in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
I want to work on this; hope it's alright.
(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.
Attached patch rrsh.patch (obsolete) — Splinter Review
The code and the two test cases: one for integers and one for objects.
Attachment #8423102 - Flags: review?(benj)
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+
I've updated my local copy but still don't see the RBitOr changes. I guess I'll wait in this case.
(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.
(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" ;)
> 
> Specifically, replace "mozilla-central" with "integration/mozilla-inbound" ;)

Awesome, thanks!
Attachment #8423102 - Attachment is obsolete: true
Attached patch rrsh.patch (obsolete) — Splinter Review
Attachment #8423249 - Flags: review?(benj)
> 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!
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+
> 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!
(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
Per discussion with Ben on IRC, he's going to run this through Try before setting checkin-needed.
Keywords: checkin-needed
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?
Attachment #8423249 - Attachment is obsolete: true
Attached patch rrsh.patchSplinter Review
Attachment #8428582 - Flags: review?(benj)
(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
Comment on attachment 8428582 [details] [diff] [review]
rrsh.patch

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

Cool, thanks!
Attachment #8428582 - Flags: review?(benj) → review+
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
Looks good to me! Can a sheriff please land it once the tree is re-opened? Thanks!
Keywords: checkin-needed
Rebased it for you, as RLsh landed in the meanwhile ;)
https://hg.mozilla.org/integration/mozilla-inbound/rev/98fb71452e0c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/98fb71452e0c
Assignee: nobody → inanc.seylan
Status: NEW → RESOLVED
Closed: 7 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.