Closed Bug 1003802 Opened 8 years ago Closed 8 years ago

IonMonkey: Implement BitNot Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

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)

Implement RBitNot in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Hi, Nicolas. I'm interested on this Bug and be assigned to.
(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.
(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
(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
Attached patch rbitnot.patch (obsolete) — Splinter Review
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)
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+
(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.
Attachment #8420658 - Attachment is obsolete: true
Attached patch rbitnot.patchSplinter Review
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)
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+
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
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.
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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.