IonMonkey: Implement Rsh Recover Instruction

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbouvier, Assigned: inanc.seylan)

Tracking

(Blocks 1 bug)

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=bbouvier][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
Implement RRsh in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Assignee

Comment 1

5 years ago
I want to work on this; hope it's alright.
Reporter

Comment 2

5 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

5 years ago
Posted 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)
Reporter

Comment 4

5 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

5 years ago
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" ;)
Assignee

Comment 8

5 years ago
> 
> Specifically, replace "mozilla-central" with "integration/mozilla-inbound" ;)

Awesome, thanks!
Assignee

Updated

5 years ago
Attachment #8423102 - Attachment is obsolete: true
Assignee

Comment 9

5 years ago
Posted patch rrsh.patch (obsolete) — Splinter Review
Attachment #8423249 - Flags: review?(benj)
Assignee

Comment 10

5 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

5 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

5 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

5 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
Per discussion with Ben on IRC, he's going to run this through Try before setting checkin-needed.
Keywords: checkin-needed
Reporter

Comment 15

5 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

5 years ago
Attachment #8423249 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
Posted patch rrsh.patchSplinter Review
Attachment #8428582 - Flags: review?(benj)
Assignee

Comment 17

5 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

5 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

5 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

5 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

5 years ago
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: 5 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.