Closed
Bug 1003802
Opened 10 years ago
Closed 10 years ago
IonMonkey: Implement BitNot Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: nbp, Assigned: inanc.seylan)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][mentor=nbp][lang=c++])
Attachments
(1 file, 1 obsolete file)
7.32 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Implement RBitNot in js/src/jit/Recover.cpp. See Bug 1003801 comment 0 for explanation.
Comment 1•10 years ago
|
||
Hi, Nicolas. I'm interested on this Bug and be assigned to.
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Rafael Vieira from comment #1) > Hi, Nicolas. I'm interested on this Bug and be assigned to. Hi, I know "ingo" started to work on this bug, so I will open another bug for MBitAnd, such as you can work on it (and CC you on it). Also our policy for new contributor is to contribute a patch before assigning a bug.
Comment 3•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #2) > (In reply to Rafael Vieira from comment #1) > > Hi, Nicolas. I'm interested on this Bug and be assigned to. > > Hi, I know "ingo" started to work on this bug, so I will open another bug > for MBitAnd, such as you can work on it (and CC you on it). > > Also our policy for new contributor is to contribute a patch before > assigning a bug. Thanks, Nicolas. I appreciate it
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to ingo from comment #1) > Created attachment 8420624 [details] [diff] [review] > rbitnot.patch > > Here's an implementation of the RBitNot recover instruction. > > I've tested it in a separate .js file that is not in the repository. The > test logic is as in dce-with-rinstructions.js. It recovers well, i.e., the > assertion is valid and I see the gray text in the generated iongraph output. Ok, add your test to dce-wit-rinstructions.js, or create another test file, and this would be ready to land in Firefox ;)
Assignee: nobody → acimilan
Assignee | ||
Comment 5•10 years ago
|
||
Ok I have resolved the 1. minor empty line issue on the constructor of RBitNot in Recover.cpp; 2. the ordering of operations in Recover.h Plus, I have added a test case for RBitNot in dce-with-rinstructions.js. I needed to modify the comments and function names a bit to be consistent. But the implementations of functions testing RAdd remain the same. Checked iongraph output and it looks fine.
Attachment #8420658 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8420658 [details] [diff] [review] rbitnot.patch Review of attachment 8420658 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, one more case to handle to avoid the furry of the security guys ;) > Bug 1003802 - added recover functionality for BitNot along with a test for it; r=:nbp nit: s/added/Add/ nit: s/ along with a test for it// nit: s/r=:nbp/r=nbp/ ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js @@ +1,5 @@ > setJitCompilerOption("baseline.usecount.trigger", 10); > setJitCompilerOption("ion.usecount.trigger", 20); > var i; > > +// Check that we are able to remove the operation inside recover test functions (denoted by "rop..."), style-nit: Remove trailing white-space. ::: js/src/jit/MIR.h @@ +3317,5 @@ > } > void computeRange(TempAllocator &alloc); > + > + bool writeRecoverData(CompactBufferWriter &writer) const; > + bool canRecoverOnBailout() const { return true; } Ok, in my previous review, I forgot one case, where we can use a bit-not on an object. We need to prevent such usage in the canRecoverOnBailout function. The getAliasSet function is similar to what we want here, as the AliasSet::None() means that the operation does not depend on the environment. The following test case should not use RBitNot, because it is not safe to manipulate objects with Recover instructions (except under special conditions). var uceFault_bitnot_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_bitnot_object')); function rbitnot_object(i) { var t = i; var o = { valueOf: function () { return t; } }; var x = ~o; /* computed with t == i, not 1000 */ t = 1000; if (uceFault_bitnot_object(i) || uceFault_bitnot_object(i)) assertEq(x, -100 /* = ~99 */); return i; } The difference of execution is due to the fact that the bitnot operation is done at the time of the second call to uceFault_bitnot_object, and not as written in the code. At this time, the object "o" still refers to the same variable "t" which is on the scope chain of the function, but the value of "t" changed from being equal to "i" to be "1000". I think we should modify the radd_object test case to be similar to this test case, as the current test case does not really check the case where we do not want to make this optimization. ::: js/src/jit/Recover.h @@ +22,2 @@ > _(Add) \ > + _(NewDerivedTypedObject) style-nit: Remove trailing white-space.
Attachment #8420658 -
Flags: review?(nicolas.b.pierron) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #6) > > ::: js/src/jit/MIR.h > @@ +3317,5 @@ > > } > > void computeRange(TempAllocator &alloc); > > + > > + bool writeRecoverData(CompactBufferWriter &writer) const; > > + bool canRecoverOnBailout() const { return true; } > > Ok, in my previous review, I forgot one case, where we can use a bit-not on > an object. We need to prevent such usage in the canRecoverOnBailout > function. The getAliasSet function is similar to what we want here, as the > AliasSet::None() means that the operation does not depend on the environment. > > The following test case should not use RBitNot, because it is not safe to > manipulate objects with Recover instructions (except under special > conditions). So, would it be enough to change the implementation of canRecoverOnBailout() to the following? return specialization_ != MIRType_None > > > var uceFault_bitnot_object = eval(uneval(uceFault).replace('uceFault', > 'uceFault_bitnot_object')); > function rbitnot_object(i) { > var t = i; > var o = { valueOf: function () { return t; } }; > var x = ~o; /* computed with t == i, not 1000 */ > t = 1000; > if (uceFault_bitnot_object(i) || uceFault_bitnot_object(i)) > assertEq(x, -100 /* = ~99 */); > return i; > } > > > The difference of execution is due to the fact that the bitnot operation is > done at the time of the second call to uceFault_bitnot_object, and not as > written in the code. At this time, the object "o" still refers to the same > variable "t" which is on the scope chain of the function, but the value of > "t" changed from being equal to "i" to be "1000". > > I think we should modify the radd_object test case to be similar to this > test case, as the current test case does not really check the case where we > do not want to make this optimization. > Yes, this test case fails.
Assignee | ||
Updated•10 years ago
|
Attachment #8420658 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
A new patch integrating the following: 1. Corrected the style nits in the commit message and dce-with-rinstructions.js 2. Updated RAdd and RBitNot test cases in dce-with-rinstructions.js for objects according to nbp's new test case. 3. Renamed some functions in dce-with-rinstructions.js to be consistent. 4. BitNot does not recover anymore when applied to an object.
Attachment #8421693 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8421693 [details] [diff] [review] rbitnot.patch Review of attachment 8421693 [details] [diff] [review]: ----------------------------------------------------------------- I wilI will update the test case locally and push this changes for you ;) ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js @@ +67,1 @@ > assertEq(x, "[object Object]99"); The assertion should be modified to assertEq(x, 198); as because of valueOf, the lhs of the plus is seeing the value returned by valueOf instead of the object.
Attachment #8421693 -
Flags: review?(nicolas.b.pierron) → review+
Reporter | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf9b36b5ebf
Reporter | ||
Comment 11•10 years ago
|
||
The previous patch appeared in our continuous integration. [1] This will run all the checks that are ran with for Firefox, and as soon as all the checks are ok, one of the Sheriff (a person which is in charge of the trees) will merge the patch in mozilla-central, and then it would ride the train and be released with Mozilla 32 ;) Nice Work :) [1] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0cf9b36b5ebf
Comment 12•10 years ago
|
||
Backed out for ggc test failures. Please make sure this passes on Try before the next push attempt :) https://hg.mozilla.org/integration/mozilla-inbound/rev/74a33ef2ec2f https://tbpl.mozilla.org/php/getParsedLog.php?id=39585389&tree=Mozilla-Inbound
Reporter | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a6c6d5621b Ok, the test case was not identical to the previous one, it was "{} + o" and not "o + i" as I had expected.
Assignee | ||
Comment 14•10 years ago
|
||
Thanks for pushing the changes and for your help in general Nicolas! It's cool that this code will make it to Mozilla 32! Sorry about this confusion with the test case. In the original ra_object test case, it was "{} + o" and I did not want to change that but just integrate your logic for rbitnot_object.
https://hg.mozilla.org/mozilla-central/rev/e7a6c6d5621b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•