1109052 - IonMonkey: Implement RToString Recover Instruction

Status: NEW

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

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[Decheol Jeong]

I would like to work on this if no one is assigned yet.

Comment 2

[Nicolas B. Pierron [:nbp]]

I will assign you on this bug as soon as you attach a patch.

Comment 3

[Fabio Buso[:SirOibaf]]

Attachment: bug1109052.patch

Hi!
This is my first bug.

I created a pach for this bug, but I'm not confident at 100% that is all right. 
Can you tell me what is wrong? 

Thanks.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8543179 [details] [diff] [review]
bug1109052.patch

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

This patch looks good, but I think the following test case will fail:


var uceFault_rtostring_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_rtostring_object'));
function rtostring_object(i) {
    var t = i;
    var o = { toString: function () { return "" + t; } };
    var x = "" + o; /* computed with t == i, not 1000 */
    t = 1000;
    if (uceFault_rtostring_object(i) || uceFault_rtostring_object(i))
        assertEq(x, "99");
    return i;
}

::: js/src/jit/MIR.h
@@ +4660,5 @@
>          return input()->mightBeType(MIRType_Object);
>      }
>  
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    

nit: remove trailing white space.

Comment 5

[Fabio Buso[:SirOibaf]]

Attachment: rtostring_number.png

Comment 6

[Fabio Buso[:SirOibaf]]

Attachment: rtostring_object.png

Comment 7

[Fabio Buso[:SirOibaf]]

I've added the rtostring_object test to the dce-with-rinstructions file.

If I understood correctly in the graph the function toString should be grey and not blue, but in both files they are blue. Did I forget something?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Fabio Buso[:SirOibaf] from comment #7)
> If I understood correctly in the graph the function toString should be grey
> and not blue, but in both files they are blue. Did I forget something?

Indeed they should be grey, but only after the DCE (Eliminate Dead Code) is done.  In which case the branches disappear and the ToString instructions are no longer used except by the resume point of the blocks.

Also, I have no idea why MCallOptimize did not inline the call to toString in rtostring_number, can you investigate as well?

Comment 9

[Till Schneidereit [:till]]

Fabio, are you still interested in finishing this?

Comment 10

[Till Schneidereit [:till]]

Unassigning after not hearing anything for a month.

Comment 11

[Abhishek Sharma]

Is this bug still valid ? If so, I would like to take a shot at fixing it.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Abhishek Sharma from comment #11)

Is this bug still valid ? If so, I would like to take a shot at fixing it.

Yes, this bug is still valid.

Comment 13

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.