1020637 - IonMonkey: Implement Concat Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[Caio Lima(:caiolima)]

Nicolas, I'm interested on working on that instruction, but I dont know about the Concat Instruction. To RAdd, RSub and etc exists an js::AddValues or js::SubValues. Another doub is, what is a Concat on javascript level? There is any MDefinition reference to help me?

Thanks sice now

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Caio Lima(:caiolima) from comment #1)
> Nicolas, I'm interested on working on that instruction, but I dont know
> about the Concat Instruction.

The concat instruction is the specialization that we have in IonBuilder[1], to distinguish between an addition which is handling any kind of values and an addition which only handle string concatenations.

Look at what case can produce this MConcat and look at how it is handled in the interpreter, and use the same function within the recover function.  By curiosity, you can also have a look at the code generator part, but the best is if we have as little assumptions as possible in the recover path.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#3849

Comment 3

[Caio Lima(:caiolima)]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> The concat instruction is the specialization that we have in IonBuilder[1],
> to distinguish between an addition which is handling any kind of values and
> an addition which only handle string concatenations.
> 
> Look at what case can produce this MConcat and look at how it is handled in
> the interpreter, and use the same function within the recover function.  By
> curiosity, you can also have a look at the code generator part, but the best
> is if we have as little assumptions as possible in the recover path.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#3849

The Interpreter already does it on 
http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter.cpp?from=vm/Interpreter.cpp:1178#1178

And It's my main confusion: Why should we implement a concat instruction, since the add already implement it?

So, Since the MConcat Exists, I will implement the RConcat using the js::AddValues either and the test cases I'll put the concatanations cases.

Say if I'm wrong, but this test case[1] won't raise the RAdd::recover(), will it?
[1] - http://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js?from=dce-with-rinstructions.js&case=true#149

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Caio Lima(:caiolima) from comment #3)
> The Interpreter already does it on 
> http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter.
> cpp?from=vm/Interpreter.cpp:1178#1178
> 
> And It's my main confusion: Why should we implement a concat instruction,
> since the add already implement it?

You are right, we can just produce a RAdd from the MConcat function.

> Say if I'm wrong, but this test case[1] won't raise the RAdd::recover(),
> will it?

I think you are right too.

Comment 5

[Caio Lima(:caiolima)]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > Say if I'm wrong, but this test case[1] won't raise the RAdd::recover(),
> > will it?
> 
> I think you are right too.

I've tested it now. My assumptions was correct. Now I have the directions to work on this. 

By the way, only in terms of undrestand the architecture, where could I find the relationship between MAdd and RAdd? I would like to know if the Baseline calls the "RDefinition" based on the "MDefinition" when the IonScript is invalidated.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Caio Lima(:caiolima) from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > > Say if I'm wrong, but this test case[1] won't raise the RAdd::recover(),
> > > will it?
> > 
> > I think you are right too.
> 
> I've tested it now. My assumptions was correct. Now I have the directions to
> work on this. 
> 
> By the way, only in terms of undrestand the architecture, where could I find
> the relationship between MAdd and RAdd?

Are you looking for Recover.cpp, or I am miss-understanding your question?

> I would like to know if the Baseline
> calls the "RDefinition" based on the "MDefinition" when the IonScript is
> invalidated.

I am not sure to understand your question.

RInstruction::recover is called when we bailout while reconstructing the Baseline stack frame.
The recover function is independent of MDefinition which was used to produce it. MDefinitions and RInstructions are not visible from Baseline point of view.

Comment 7

[Caio Lima(:caiolima)]

Forget my questions, I'm reading the code and I found the writeRecoverData method. So, It clarified me wich recover instruction wil be called.

Comment 8

[Caio Lima(:caiolima)]

Attachment: 1003801.patch

It's my first proposal. I don't understand the function of canBailsout().

An important point is that this RInstruction is much like RAdd. So, maybe It's not necessary to create the RConcat, but use the RInstruction::Recover_Add on MConcat::writeRecoverData. I don't think that it's good for code maintenance.

Comments and another suggestions.

Comment 9

[Caio Lima(:caiolima)]

Comment on attachment 8435061 [details] [diff] [review]
1003801.patch

I guess I used wrong flag

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Caio Lima(:caiolima) from comment #8)
> Created attachment 8435061 [details] [diff] [review]
> 1003801.patch
> 
> It's my first proposal. I don't understand the function of canBailsout().

canRecoverOnBailout is used to remove the instruction when it has no more uses except the one from the resume point.  See jit::EliminateDeadCode. [1]

> An important point is that this RInstruction is much like RAdd. So, maybe
> It's not necessary to create the RConcat, but use the
> RInstruction::Recover_Add on MConcat::writeRecoverData. I don't think that
> it's good for code maintenance.

I have no strong opinion on this point yet. Either way is fine for me, so do the one that you prefer.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#191

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8435061 [details] [diff] [review]
1003801.patch

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

This is good :)
A few style issues and we can test & land this patch on Mozilla code base ;)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +211,5 @@
> +var uceFault_concat_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_concat_float'));
> +function rconcat_float(i) {
> +    var x = "s" + i.toFixed(2);
> +    if (uceFault_concat_float(i) || uceFault_concat_float(i))
> +        assertEq(x, "s99.00");

There is no "float" case which differs from rconcat_number.
This test case is equivalent to rconcat_string.

::: js/src/jit/Recover.cpp
@@ +379,5 @@
> +RConcat::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +	RootedValue lhs(cx, iter.read());
> +	RootedValue rhs(cx, iter.read());
> +	RootedValue result(cx);

style-nit: Do not use tab.

One way for me to notice tab in my editors is by settings a large & odd number of spaces associated with a tab.

Comment 12

[Caio Lima(:caiolima)]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> Comment on attachment 8435061 [details] [diff] [review]
> 1003801.patch
> 
> Review of attachment 8435061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is good :)
> A few style issues and we can test & land this patch on Mozilla code base ;)
> 
> ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> @@ +211,5 @@
> > +var uceFault_concat_float = eval(uneval(uceFault).replace('uceFault', 'uceFault_concat_float'));
> > +function rconcat_float(i) {
> > +    var x = "s" + i.toFixed(2);
> > +    if (uceFault_concat_float(i) || uceFault_concat_float(i))
> > +        assertEq(x, "s99.00");
> 
> There is no "float" case which differs from rconcat_number.
> This test case is equivalent to rconcat_string.

Do you mean "equivalent to rconcat_number"?

> ::: js/src/jit/Recover.cpp
> @@ +379,5 @@
> > +RConcat::recover(JSContext *cx, SnapshotIterator &iter) const
> > +{
> > +	RootedValue lhs(cx, iter.read());
> > +	RootedValue rhs(cx, iter.read());
> > +	RootedValue result(cx);
> 
> style-nit: Do not use tab.
> 
> One way for me to notice tab in my editors is by settings a large & odd
> number of spaces associated with a tab.

Sorry, I'm not use to use tabs, but I've changed my editor and forget to configure it. Anyway, Thank you for review!

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Caio Lima(:caiolima) from comment #12)
> (In reply to Nicolas B. Pierron [:nbp] from comment #11)
> > There is no "float" case which differs from rconcat_number.
> > This test case is equivalent to rconcat_string.
> 
> Do you mean "equivalent to rconcat_number"?

Yes, this is what I meant.

Comment 14

[Caio Lima(:caiolima)]

Attachment: 1003801.patch

Tips implemented, and rebased code

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8438151 [details] [diff] [review]
1003801.patch

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

Awesome :)
I will send this patch to our continuous integration infrastructure, and as soon as it verified there, I will land this patch in Firefox code base ;)

::: js/src/jit/Recover.h
@@ +25,5 @@
>      _(Rsh)                                      \
>      _(Ursh)                                     \
>      _(Add)                                      \
>      _(Sub)                                      \
> +    _(Concat)                                   \

I will move it below to follow the same order as in Bug 1003801 comment 0.

Comment 16

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&rev=aed895e39105

Comment 17

[Nicolas B. Pierron [:nbp]]

The previous failure was caused by Bug 1015498 that I tested in the same Try push.
The following push fix the build issue, and highlight that the current patch is good for landing on inbound.

https://tbpl.mozilla.org/?tree=Try&rev=405aa00577a8

Comment 18

[Nicolas B. Pierron [:nbp]]

Congratulation! :)

You can see your patch[hg] being verified on our test infrastructure[tbpl].  Then a sheriff will take mozilla-inbound and merge it in mozilla-central, which is used for compiling Nightly builds of Firefox ;)

[hg] https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5756b0da28
[tbpl] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3b5756b0da28

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3b5756b0da28

Comment 20

[Caio Lima(:caiolima)]

(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> Congratulation! :)
> 
> You can see your patch[hg] being verified on our test infrastructure[tbpl]. 
> Then a sheriff will take mozilla-inbound and merge it in mozilla-central,
> which is used for compiling Nightly builds of Firefox ;)
> 
> [hg] https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5756b0da28
> [tbpl] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3b5756b0da28
 Thank tou!

Comment 21

[Nicolas B. Pierron [:nbp]]

Caio, if you want you can either continue on other Recover instructions[1], or take over a string optimization[2] ;)

[1] Bug 1020638
[2] Bug 977966