Closed
Bug 1010339
Opened 10 years ago
Closed 10 years ago
IonMonkey: Implement Ursh 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, 1 obsolete file)
5.03 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
Implement RUrsh in js/src/jit/Recover.cpp. See Bug 1003801 comment 0 for explanation.
Assignee | ||
Comment 1•10 years ago
|
||
Recover for Ursh (>>>) and two test cases (number and object).
Attachment #8423781 -
Flags: review?(benj)
Reporter | ||
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
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?
Reporter | ||
Comment 4•10 years ago
|
||
(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 ;)
Assignee | ||
Updated•10 years ago
|
Attachment #8423781 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8423836 -
Flags: review?(benj)
Assignee | ||
Comment 6•10 years ago
|
||
> 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.
Reporter | ||
Comment 7•10 years ago
|
||
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+
Reporter | ||
Comment 8•10 years ago
|
||
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: 10 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.
Description
•