Closed Bug 1010339 Opened 5 years ago Closed 5 years ago

IonMonkey: Implement Ursh Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

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, 1 obsolete file)

Implement RUrsh in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Attached patch rursh.patch (obsolete) — Splinter Review
Recover for Ursh (>>>) and two test cases (number and object).
Attachment #8423781 - Flags: review?(benj)
Comment on attachment 8423781 [details] [diff] [review]
rursh.patch

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

Thanks for doing that one too! I would just like to see the tests comments addressed and we'll be good to go!

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +70,5 @@
>  }
>  
> +var uceFault_ursh_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_ursh_number'));
> +function rursh_number(i) {
> +    var x = -i >>> 1;

nit: you're testing two things here: the minus operation and then the ursh. I'd rather test only one thing at a time. You can use i >>> 1; and then test for x == 48. However, I guess you've done this as the result of i >>> 1 is the same for i == 98 and i == 99; if that was your point, you can assert in the if body below that i == 99.

@@ +78,5 @@
> +}
> +
> +var uceFault_ursh_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_ursh_object'));
> +function rursh_object(i) {
> +    var t = -i;

Same here.
Attachment #8423781 - Flags: review?(benj) → feedback+
Hi,

> nit: you're testing two things here: the minus operation and then the ursh.
> I'd rather test only one thing at a time. You can use i >>> 1; and then test
> for x == 48. However, I guess you've done this as the result of i >>> 1 is
> the same for i == 98 and i == 99; if that was your point, you can assert in
> the if body below that i == 99.
> 

I was reading this page:
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Bitwise_Operators

Since x >> y and x >>> y give the same result for x >= 0, I wanted to explicate the difference between the two operators using a negative x. 

You are right though, the test is not isolated due to the minus operator. Perhaps, we can add another for loop that counts down to -99 from 0 (instead of up as in other test cases) and put the test for >>> inside that loop?
(In reply to Inanc Seylan from comment #3)
> Since x >> y and x >>> y give the same result for x >= 0, I wanted to
> explicate the difference between the two operators using a negative x. 
> 
> You are right though, the test is not isolated due to the minus operator.
> Perhaps, we can add another for loop that counts down to -99 from 0 (instead
> of up as in other test cases) and put the test for >>> inside that loop?

Although there is no real risk of confusion between the >> and >>> tests (as they are well isolated in different functions), I think the loop with negative indexes (from 0 to -100 excluded, i.e. using i--) brings other benefits: it can help distinguish results for i == -99 and i == -98, for both >> and >>> tests. If you do that, you'll need to write a copy of uceFault that gets triggered for the right value. 
I am still fine with a simple test as explained in the last review, but feel free to implement the negative iterating loop if you want ;)
Attachment #8423781 - Attachment is obsolete: true
Attached patch rursh.patchSplinter Review
Attachment #8423836 - Flags: review?(benj)
> I am still fine with a simple test as explained in the last review, but feel
> free to implement the negative iterating loop if you want ;)

Ok, let's go for the easy fix this time ;) I've uploaded the new patch.
Comment on attachment 8423836 [details] [diff] [review]
rursh.patch

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

Cool, thanks! I've pushed it to try for you.
https://tbpl.mozilla.org/?tree=Try&rev=2b47c405ae8b

If you plan on keeping on contributing, could you create a bug for obtaining level 1 commit access, CC nbp and me, so that you can push on the try server by yourself? See http://www.mozilla.org/hacking/commit-access-policy/ for detailed instructions.
Attachment #8423836 - Flags: review?(benj) → review+
All green, let's land it! Thanks again for your patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a298fb5be4e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a298fb5be4e
Assignee: nobody → inanc.seylan
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.