IonMonkey: Implement Concat Recover Instruction

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nbp, Assigned: caiolima)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Implement RConcat in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
(Reporter)

Updated

3 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

3 years ago
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
(Reporter)

Comment 2

3 years ago
(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
(Assignee)

Comment 3

3 years ago
(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
(Reporter)

Comment 4

3 years ago
(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.
(Assignee)

Comment 5

3 years ago
(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.
(Reporter)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

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

Comment 8

3 years ago
Created attachment 8435061 [details] [diff] [review]
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.
Attachment #8435061 - Flags: feedback?(nicolas.b.pierron)
(Assignee)

Comment 9

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

I guess I used wrong flag
Attachment #8435061 - Flags: feedback?(nicolas.b.pierron) → review?(nicolas.b.pierron)
(Reporter)

Comment 10

3 years ago
(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
(Reporter)

Comment 11

3 years ago
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.
Attachment #8435061 - Flags: review?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 12

3 years ago
(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!
(Reporter)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
Created attachment 8438151 [details] [diff] [review]
1003801.patch

Tips implemented, and rebased code
Attachment #8435061 - Attachment is obsolete: true
Attachment #8438151 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 15

3 years ago
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.
Attachment #8438151 - Flags: review?(nicolas.b.pierron) → review+
(Reporter)

Comment 16

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=aed895e39105
(Reporter)

Comment 17

3 years ago
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
(Reporter)

Comment 18

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/3b5756b0da28
Assignee: nobody → ticaiolima
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 20

3 years ago
(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!
(Reporter)

Comment 21

3 years ago
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
You need to log in before you can comment on or make changes to this bug.