Closed Bug 1013821 Opened 8 years ago Closed 8 years ago

IonMonkey: Implement Not Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bbouvier, Assigned: layus, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 3 obsolete files)

Implement RNot in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

Be aware it's not BitNot (i.e. ~x) but the boolean Not (!x), which tests for truthiness.
I'll take this one if you want !
(In reply to Julien Levesy from comment #1)
> I'll take this one if you want !

Don't you want to continue on the Math.floor optimization?
I'd like to take this one and work on it. It's my first time here and I guess this is an easy one by the looks of it. Can someone assign me please?
(In reply to Anirudh Vemula from comment #3)
> I'd like to take this one and work on it. It's my first time here and I
> guess this is an easy one by the looks of it. Can someone assign me please?

Welcome :)

we do not assign bugs anymore until the first contributor contribute his first patch, to avoid keeping good first bugs locked away from new contributors.

Feel free to come back here or on IRC (irc.mozilla.org #jsapi) if you have any questions.
Oh, okay! I didn't realize it was already being tackled by someone else. So, how do I know once the first contributor submits his first patch and I can take it on?
Sorry for the confusion. Ignore my previous comment. I misunderstood what you said.

So, I have to first submit a patch and only then I can get assigned to bugs. Is that right?

(In reply to Anirudh Vemula from comment #5)
> Oh, okay! I didn't realize it was already being tackled by someone else. So,
> how do I know once the first contributor submits his first patch and I can
> take it on?
(In reply to Anirudh Vemula from comment #6)
> So, I have to first submit a patch and only then I can get assigned to bugs.
> Is that right?

That's right.
 Anirudh Vemula, I'm interested on working on this bug. Are you going to submit a patch yet?
(In reply to Caio Lima(:caiolima) from comment #8)
>  Anirudh Vemula, I'm interested on working on this bug. Are you going to
> submit a patch yet?

If you want, I've opened Bug 1020637 for you ;)
Mentor: benj
Whiteboard: [good first bug][mentor=bbouvier][lang=c++] → [good first bug][lang=c++]
Hi Anirudh Vemula, are you still working on this bug? No pressure, but if you don't have time these days, other contributors might be interested by this bug too ;) If you need any kind of help, don't hesitate to ask here, by email or on IRC (irc.mozilla.org #jsapi or #ionmonkey).
Flags: needinfo?(vvanirudh)
Hey, I am extremely sorry for not replying early. I didn't get any notification. I couldn't work on the bug due to other constraints. Please, declare it as open to other contributors. 

Once again, I am sorry for replying so late
Flags: needinfo?(vvanirudh)
(In reply to Anirudh Vemula from comment #11)
> Hey, I am extremely sorry for not replying early. I didn't get any
> notification. I couldn't work on the bug due to other constraints. Please,
> declare it as open to other contributors. 
> 
> Once again, I am sorry for replying so late

No problem at all, feel free to come back whenever you want ;)

In the meanwhile, anybody can take on this bug by posting a patch. I am here if you need further help or have any questions.
I will try to work on this.
Hi Nicolas, did you already start working on this ?
If not, I may be interested in taking this issue myself, as I had a look in the code for that part during the squashing party.
Flags: needinfo?(devillers.nicolas)
Hi guillaume, I didn't start yet so feel free to take this issue :)
Flags: needinfo?(devillers.nicolas)
Attached patch dce-not-recover.diff (obsolete) — Splinter Review
This has been carefully reviewed.
$ python2 jit-test/jit_test.py -j4 build-debug/dist/bin/js 
[4199|   0|   0|   0] 100% ==========================================>| 649.7s
PASSED ALL

nit: Some function calls in Recover.cpp are not prefixed with js:: (including the call to ToBoolean in this atch). Is this important ?
Attachment #8446118 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8446118 [details] [diff] [review]
dce-not-recover.diff

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

Stealing review, as I initially opened the bug.
That's good :) but the Not operator doesn't behave like the others, so you'll need slightly different tests and add a few lines of code.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +283,5 @@
> +var uceFault_not_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_not_number'));
> +function rnot_number(i) {
> +    var x = !i;
> +    if (uceFault_not_number(i) || uceFault_not_number(i))
> +        assertEq(x, !i);

Could you please replace !i by "false" in this condition and then add a comment stating that !99 is false?

@@ +292,5 @@
> +function rnot_object(i) {
> +    var t = i;
> +    var o = { valueOf: function() { return t; } };
> +    var x = !o; /* computed with t == i, not !i */
> +    t = !i;

The Not operator doesn't behave like others: when an object flows in, we don't use valueOf to have the truthiness value of the object. Instead, almost all objects o are such that !o is false (even empty objects). The only objects that are not, are the ones which are emulating undefined [1]. Objects which don't certainly emulate undefined are just constant folded into false during GVN [2]. So your test could be way simpler: you can just create an object with "objectEmulatingUndefined()" (that's a shell specific function that will create such an edge-case object) and then adapt the assertEq (no more need for t).

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#1068 calls http://dxr.mozilla.org/mozilla-central/source/js/src/jsbool.cpp#185 for objects
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#2715

::: js/src/jit/MIR.h
@@ +6240,5 @@
>          return congruentIfOperandsEqual(ins);
>      }
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool canRecoverOnBailout() const {
> +        return true;

See comment below.

::: js/src/jit/Recover.cpp
@@ +499,5 @@
> +{
> +    RootedValue v(cx, iter.read());
> +    RootedValue result(cx);
> +
> +    MOZ_ASSERT(!v.isObject());

There is nothing that prevents objects from flowing in, here, since MNot's type policy (which converts inputs to make sure that they fit the MInstruction needs) allows Objects. But as stated before, only objects that might emulate undefined can flow in. You need to modify the canRecoverOnBailout function so that it tests for its member operandMightEmulateUndefined_, which is computed way before we consider eliminating the instruction, thus is safe. Then this assert will be correct.
Attachment #8446118 - Flags: review?(nicolas.b.pierron) → feedback+
> Stealing review, as I initially opened the bug.

No problem. I ask reviews from :nbp because I met him at the squashing party in Paris, but any review is fine.

> The Not operator doesn't behave like others: when an object flows in, we
> don't use valueOf to have the truthiness value of the object. Instead,
> almost all objects o are such that !o is false (even empty objects). The
> only objects that are not, are the ones which are emulating undefined [1].
> Objects which don't certainly emulate undefined are just constant folded
> into false during GVN [2]. So your test could be way simpler: you can just
> create an object with "objectEmulatingUndefined()" (that's a shell specific
> function that will create such an edge-case object) and then adapt the
> assertEq (no more need for t).
> 
> [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#1068 calls
> http://dxr.mozilla.org/mozilla-central/source/js/src/jsbool.cpp#185 for
> objects
> [2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#2715
> 
> [...]
> 
> There is nothing that prevents objects from flowing in, here, since MNot's
> type policy (which converts inputs to make sure that they fit the
> MInstruction needs) allows Objects. But as stated before, only objects that
> might emulate undefined can flow in. You need to modify the
> canRecoverOnBailout function so that it tests for its member
> operandMightEmulateUndefined_, which is computed way before we consider
> eliminating the instruction, thus is safe. Then this assert will be correct.

If I understand well, we can always recover the Not operator.
If it is true, then nothing needs to be checked in Recover.{h,cpp} and function rnot_object is useless.

Is there something special about "objectEmulatingUndefined()" that makes the Not operator unrecoverable ?
Flags: needinfo?(benj)
(In reply to Guillaume Maudoux from comment #18)
> If I understand well, we can always recover the Not operator.
Unfortunately, that is not true. We shouldn't try to recover objects (currently). However, there is only one way Not operations on objects don't get eliminated, and it's for objects that might emulate undefined (as !undefined === true).

To summarize, there are only two cases:
- either the input is an object that might emulate undefined (i.e. the type of MNot is object and operandMightEmulateUndefined_ is true) => it can flow in MNot and won't be replaced by false => it is seen as an Object when we decide whether or not it can be recovered => it shouldn't be recovered, as we don't recover objects.
- or the input is an object which won't emulate undefined for sure (i.e. the type of MNot is object and operandMightEmulateUndefined_ is false) => during GVN, the MNot operation will get folded into a boolean value set to false (link [2] in my previous comment) => no more MNot operation, we won't call canRecoverOnBailout at all.
- or the input isn't an object, we can recover it on bailout.

As a matter of fact, in canRecoverOnBailout, you need to test for the type() and operandMightEmulateUndefined_ and choose accordingly.

> If it is true, then nothing needs to be checked in Recover.{h,cpp} and
> function rnot_object is useless.
You need to keep the test for rnot_object and use an object created by objectEmulatingUndefined(), so that !o is true (if o is the name of the object). The assertion in Recover.cpp can be kept as well.
 
Sorry it's more difficult than expected, as that's a pretty weird edge case, but that's also an opportunity to learn more how it works :) Don't hesitate to ask if you have more questions, here or on IRC (on #jsapi or #ionmonkey, my nick is bbouvier).
Flags: needinfo?(benj)
(In reply to Benjamin Bouvier [:bbouvier] from comment #19)
> > If I understand well, we can always recover the Not operator.
> Unfortunately, that is not true. We shouldn't try to recover objects
> (currently). However, there is only one way Not operations on objects don't
> get eliminated, and it's for objects that might emulate undefined (as
> !undefined === true).

Does it means that we can always recover the MNot operation, but for some reason we prefer to avoid recovering operations on objects ?

From a design point of view, the fact the we do not want to recover operations on objects should be coded elsewhere, for example around http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#191. Maybe something like :
-        } else if (!inst->hasLiveDefUses() && inst->canRecoverOnBailout()) {
+        } else if (!inst->hasLiveDefUses() && inst->canRecoverOnBailout() && !inst->isPerformedOnObject()) {
If implemented like this, then the MNot operation can always recover.

Of course, it may be easier to just put that check in MNot. In this case, I do not know which of the following two implementations you prefer.

1/ Avoid objects. This covers the unwanted case of objects emulating undefined, and the impossible case of other objects. 
> +    bool canRecoverOnBailout() const {
> +        /* not sure if this is valid as I have no compiler right now.
> +         * I am sure you get the idea :-) */
> +        return ins->type() < MIRType_Object;
> +    }

2/ Avoid only the case of objects that may emulate undefined, as the other is impossible.
> +    bool canRecoverOnBailout() const {
> +        return !operandMightEmulateUndefined_;
> +    }

My preference goes to the fist option, as it shows clearly that we do not support objects (for now).
The second option seems to hint that there is something special about "operandMightEmulateUndefined_" that needs to be checked; but there is none, as we can recover MNot on objects that may emulate undefined as soon as we can recover operations on objects.

> > If it is true, then nothing needs to be checked in Recover.{h,cpp} and
> > function rnot_object is useless.
> You need to keep the test for rnot_object and use an object created by
> objectEmulatingUndefined(), so that !o is true (if o is the name of the
> object). The assertion in Recover.cpp can be kept as well.

OK.

> Sorry it's more difficult than expected, as that's a pretty weird edge case,
> but that's also an opportunity to learn more how it works :) Don't hesitate
> to ask if you have more questions, here or on IRC (on #jsapi or #ionmonkey,
> my nick is bbouvier).

I do not feel scared off by the difficulty, it must be a ggod sign :-).
If I did not get it right with this post, I will surely contact you on irc.
Thanks for your detailed reviews.
Flags: needinfo?(benj)
(In reply to Guillaume Maudoux from comment #20)
> Does it means that we can always recover the MNot operation, but for some
> reason we prefer to avoid recovering operations on objects ?
Yes, but that's only *for now*. In the future, we may want to recover operations on objects as well.

> 
> From a design point of view, the fact the we do not want to recover
> operations on objects should be coded elsewhere, for example around
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#191.
> Maybe something like :
> -        } else if (!inst->hasLiveDefUses() && inst->canRecoverOnBailout()) {
> +        } else if (!inst->hasLiveDefUses() && inst->canRecoverOnBailout()
> && !inst->isPerformedOnObject()) {
> If implemented like this, then the MNot operation can always recover.
So far, any time an operation couldn't be recovered because it was specialized as Object, this information was contained in canRecoverOnBailout. Having isPerformedOnObject would prevent future optimizations as well and would need to add back specializations in all the canRecoverOnBailout methods, so let's keep it that way.

> 
> The second option seems to hint that there is something special about
> "operandMightEmulateUndefined_" that needs to be checked; but there is none,
> as we can recover MNot on objects that may emulate undefined as soon as we
> can recover operations on objects.

Second option is the one I expect, as it lets room to improvement for the future. Feel free to add a comment explaining why we're checking that, there.

> I do not feel scared off by the difficulty, it must be a ggod sign :-).
> If I did not get it right with this post, I will surely contact you on irc.
> Thanks for your detailed reviews.
No problem, thanks for keeping on working on it :) don't hesitate to come and say hi on irc!
Flags: needinfo?(benj)
Attached patch dce-not-recover.diff (obsolete) — Splinter Review
I made a "huge" comment in the code. I am not sure that a bug number in the code is a good idea, but it should not hurt.
Attachment #8446118 - Attachment is obsolete: true
Attachment #8447357 - Flags: review?(benj)
Attached patch dce-not-recover.diff (obsolete) — Splinter Review
Fixed extra space.
Attachment #8447357 - Attachment is obsolete: true
Attachment #8447357 - Flags: review?(benj)
Attachment #8447424 - Flags: review?(benj)
Comment on attachment 8447424 [details] [diff] [review]
dce-not-recover.diff

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

Thanks a lot! Please fix the last comments, make sure JIT tests pass locally and I'll help you have this landing. Cheers and congratulations for this patch! (no need to ask again for review once you upload the new version, you can just set the r+ by yourself on the newly attached patch)

::: js/src/jit/MIR.h
@@ +6241,5 @@
>      }
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool canRecoverOnBailout() const {
> +        // Non objects are recoverable and objects that cannot emulate
> +        // undefined have a constant truth value of 'true'.

s/have a constant truth value of 'true'/get folded into 'true' by GVN.

@@ +6245,5 @@
> +        // undefined have a constant truth value of 'true'.
> +        // So the only way to reach this function with an operand that
> +        // is an object is when that object might emulate undefined.
> +        // For now, we do not want to recover operations on objects.
> +        // See Bug 1013821 comment 19 and following.

Nice, can you remove the last two lines please (from "For now" to "and following").

::: js/src/jit/Recover.cpp
@@ +499,5 @@
> +{
> +    RootedValue v(cx, iter.read());
> +    RootedValue result(cx);
> +
> +    result.setBoolean(!ToBoolean(v));

nit: as we can't have objects flowing in here, how about reintroducing the MOZ_ASSERT(!v.isObject());
Attachment #8447424 - Flags: review?(benj) → review+
Attachment #8447424 - Attachment is obsolete: true
Attachment #8448517 - Flags: review+
Thank you for this contribution! Have you found another bug on which you'd like to work on?

https://hg.mozilla.org/integration/mozilla-inbound/rev/881e7b3d101f
Assignee: nobody → layus.on
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/881e7b3d101f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.