1062893 - IonMonkey: Implement RCompare Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[gokcehankara]

Nicolas, it sounds interesting, I can take a shot at this.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to gokcehankara from comment #1)
> Nicolas, it sounds interesting, I can take a shot at this.

Sure ;)

Comment 3

[gokcehankara]

Attachment: 1062893.patch - IonMonkey: Implement RCompare Recover Instruction

Looks like it has already been over 3 weeks since I started working on this and I'm still not 100% sure what needs to be done here, so I thought I should ask for some feedback before I get lost completely.

So I have read the blog post (https://blog.mozilla.org/javascript/2014/07/15/ionmonkey-optimizing-away/) and got a vague idea about what recover instructions are for, although I still can't interpret the images in the blog post. I also learned how to generate iongraph images which seems quite useful.

I'm attaching a patch that I have assembled by cheating from other people's patches along with the generated iongraph image at DCE-mir pass. I have also put together a list of stupid questions for starter below.

1) What is a compare instruction? (`==`, `===`, `<`, `>`, `<=`, `>=` or maybe some assembly like instruction at a lower level?)
1.1) I have currently implemented it using `js::LooselyEqual`. Assuming that I'm on the right track, do I need to implement other comparisons and test cases for comparing different types?
2) What should I be looking for in iongraph images? (At first I thought I should check that every BBL should have resume points (gray) but some of mine doesn't. Is this a problem?)
3) I have added test cases and other classes for the recover instruction just before the add instruction, is there a particular order of recover instructions in the code?

Comment 4

[gokcehankara]

Attachment: func00-pass15-DCE-mir.gv.png

This is the iongraph image for DCE-mir phase generated with the attached patch applied.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to gokcehankara from comment #3)
> Looks like it has already been over 3 weeks since I started working on this
> and I'm still not 100% sure what needs to be done here, so I thought I
> should ask for some feedback before I get lost completely.

This is a good way to go when you are blocked ;)

> So I have read the blog post
> (https://blog.mozilla.org/javascript/2014/07/15/ionmonkey-optimizing-away/)
> and got a vague idea about what recover instructions are for, although I
> still can't interpret the images in the blog post. I also learned how to
> generate iongraph images which seems quite useful.

Maybe these svg would help you understand the relation between IonGraph and Recover instructions:
https://nbp.github.io/slides/RInstruction/index.html

> I'm attaching a patch that I have assembled by cheating from other people's
> patches along with the generated iongraph image at DCE-mir pass. I have also
> put together a list of stupid questions for starter below.

I have not yet look at the patch, but the first "compare" show in gray in the image, which means that it is no longer being used to produce code, but that it would be used in case of a bailout.

> 1) What is a compare instruction? (`==`, `===`, `<`, `>`, `<=`, `>=` or
> maybe some assembly like instruction at a lower level?)

This is used to represent all JavaScript comparison relations.  The "op" fields correspond to the bytecode for the instruction which is represented by MCompare.  You can see the list in Opcodes.h

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Opcodes.h#244-249,593-594

and we construct it from IonBuilder.cpp

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#1495-1503

> 1.1) I have currently implemented it using `js::LooselyEqual`. Assuming that
> I'm on the right track, do I need to implement other comparisons and test
> cases for comparing different types?

Yes, as long as these are not mutable objects.  Currently we do not support mutable objects as their content might change.  So if we remove and instruction to execute it later on the bailout path, we still want to be computing the same result.  Basically, this is fine for numbers and string.

> 2) What should I be looking for in iongraph images? (At first I thought I
> should check that every BBL should have resume points (gray) but some of
> mine doesn't. Is this a problem?)

You should look at the instruction 10, this MCompare is in gray, which mean that the instruction is flagged as recovered on bailout :)  Other MCompare are coming from the inlined function.  You can recognize inlined functions because their resume points are annotated with "((<block-index>))", the end of the block numbered "block-index" corresponds to the location where the function is being inlined.

> 3) I have added test cases and other classes for the recover instruction
> just before the add instruction, is there a particular order of recover
> instructions in the code?

So far, we tried to follow the order listed in Bug 1003801, as this list group everything by semantic of the instructions.  I should probably add comment above each sections later …

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8511636 [details] [diff] [review]
1062893.patch - IonMonkey: Implement RCompare Recover Instruction

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

This patch looks good so far.

Now, you need to transfer the JSOp field to the recover instruction, such as we know what kind of operation should be recovered.
The assertion of the following test will fail until you can distinguish between == and != in the recover function.

>	var uceFault_compare_ne_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_compare_ne_number'));
>	function rcompare_ne_number(i) {
>	    var x = 1 != i;
>	    if (uceFault_compare_ne_number(i) || uceFault_compare_ne_number(i))
>	        assertEq(x, true /* = 1 != 99 */);
>	    return i;
>	}

::: js/src/jit/MIR.h
@@ +3324,5 @@
>  # endif
>  
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool canRecoverOnBailout() const {
> +        return true;

This is the location where you want to prevent using non number / string comparisons.

::: js/src/jit/Recover.cpp
@@ +327,5 @@
>  bool
> +MCompare::writeRecoverData(CompactBufferWriter &writer) const
> +{
> +    MOZ_ASSERT(canRecoverOnBailout());
> +    writer.writeUnsigned(uint32_t(RInstruction::Recover_Compare));

have a look at writeByte to encode the JSOp, and to decode it in the constructor.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1062893: Add recover support for MCompare. r=iain!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d6c12570af3
Add recover support for MCompare. r=iain

Comment 10

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/4d6c12570af3