Closed Bug 1020637 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement Concat Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: nbp, Assigned: caiolima)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Implement RConcat in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
OS: Linux → All
Hardware: x86_64 → All
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
(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
(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
(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.
(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.
(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.
Forget my questions, I'm reading the code and I found the writeRecoverData method. So, It clarified me wich recover instruction wil be called.
Attached patch 1003801.patch (obsolete) — Splinter Review
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)
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)
(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 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+
(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!
(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.
Attached patch 1003801.patchSplinter Review
Tips implemented, and rebased code
Attachment #8435061 - Attachment is obsolete: true
Attachment #8438151 - Flags: review?(nicolas.b.pierron)
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+
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
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(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!
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.