872045 - IonMonkey: Add MParseInt

Status: RESOLVED DUPLICATE of bug 815255

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Follow-up for bug 872020. This would allow us to hoist it out of loops and enable some other optimizations.

Comment 1

[Nicolas B. Pierron [:nbp]]

I think I know the parseInt that you are thinking of in PDFJS, but sadly I don't think we might optimize it much for now as the sad fact is that the parseInt which in in this huge function, see a bunch of crap which is not always starting with digit, and so return NaN as expected.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Add MParseInt

This is the same patch as bug 872020, but now adding MParseInt. (I'll probably close bug 872020 as duplicated of this one).

I did not move the constant folding to GVN, since that can fail (OOM) and we need cx, which is atm not available in GVN...
Also I now created 1 patch. I can split it again, if needed ...

Improves pdf.js: 12000 -> 19400

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> I did not move the constant folding to GVN, since that can fail (OOM) and we
> need cx, which is atm not available in GVN...

I am not sure to understand, is the LIFO too small?

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I am not sure to understand, is the LIFO too small?

No. The num_parseInt function to compute the constant value out of the inputs can OOM and needs cx ;)

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 751014 [details] [diff] [review]
Add MParseInt

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

::: js/src/ion/MCallOptimize.cpp
@@ +1392,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    if (callInfo.argc() > 2)
> +        return InliningStatus_NotInlined;
> +

Here we should check that callInfo.getArg(0)->type() == MIRType_String, and abort inlining otherwise. Same for the radix argument (ensure it's MIRType_Int32 if present). Else it could be some object with toString/valueOf, and we could call into JS during compilation (and it would be invalid to treat MParseInt as idempotent).

Furthermore, you can change MParseInt's type policy to take String + Int32, and change the VM call to take unboxed values. That should simplify the generated code a bit.

A more annoying issue is that parseInt can return either an int32 or double value. If getInlineReturnType() == MIRType_Int32, and we see a double, we have to invalidate somehow. In the constant-arguments case, you can just check for this and abort. If the arguments are not constant and getInlineReturnType() == MIRType_Int32, the easiest thing to do is to just add a type barrier like inlineArrayPopShift.

@@ +1396,5 @@
> +
> +    callInfo.unwrapArgs();
> +
> +    // ParseInt without arguments
> +    if (callInfo.argc() == 0) {

It's simpler to move this check up and just abort.

::: js/src/ion/MIR.cpp
@@ +195,5 @@
> +    if (isUnbox())
> +        return toUnbox()->getOperand(0)->isConstantValue();
> +
> +    if (isBox())
> +        return toBox()->getOperand(0)->isConstantValue();

Shouldn't we instead fold MBox/MUnbox with a constant input?

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Add MParseInt

Removed the MUnbox case from isConstantValue. It indeed makes no sense and haven't encounter it. I did see the MBox coming by a long time ago... So I'll keep that.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 752685 [details] [diff] [review]
Add MParseInt

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

Keeps the posted score of the previous patch ;)

::: js/src/ion/MCallOptimize.cpp
@@ +1450,5 @@
> +
> +    // When no doubles are seen yet, need to monitor them.
> +    bool needsBarrier = returnType == MIRType_Int32;
> +    if (!pushTypeBarrier(parseInt, getInlineReturnTypeSet(), needsBarrier))
> +        return InliningStatus_Error;

The following is also correct:

if (returnType == MIRType_Int32) {
    if (!pushTypeBarrier(parseInt, getInlineReturnTypeSet(), needsBarrier))
        return InliningStatus_Error;
}

but I think the original has better type information?

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 752685 [details] [diff] [review]
Add MParseInt

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

Nice :)

::: js/src/ion/MCallOptimize.cpp
@@ +1426,5 @@
> +
> +       callInfo.unwrapArgs();
> +       if (callInfo.argc() == 1) {
> +           JS_ASSERT(radix->isConstant());
> +           current->add(radix->toConstant());

You can remove this, we folded the call so the radix MIR instruction has no uses.

@@ +1449,5 @@
> +    current->push(parseInt);
> +
> +    // When no doubles are seen yet, need to monitor them.
> +    bool needsBarrier = returnType == MIRType_Int32;
> +    if (!pushTypeBarrier(parseInt, getInlineReturnTypeSet(), needsBarrier))

To answer your comment, this looks fine :)

::: js/src/ion/VMFunctions.cpp
@@ +795,5 @@
> +    AutoValueArray ava(cx, argv, 4);
> +    if (!js::num_parseInt(cx, 2, argv))
> +        return false;
> +
> +    JS_ASSERT(argv[0].isDouble() || argv[0].isInt32());

Nit: isNumber()

Comment 10

[Hannes Verschore [:h4writer]]

Argh, just when I get r+, I find an issue with the patch:

function foo5(n) {
  var b = 0;
  for (var i = 0; i < n; i++) {
    b = parseInt("10", i);
    print(b);
  }
}
foo5(2)
foo5(5)

This will start outputting fixed numbers, instead of the right numbers. This is caused by the optimization of phi with a single operand, that is incorrect. In IonBuilder that means we still need to find the second+ operands to add in the Phi. So the reported improvement is bogus :(. I even don't see a performance difference w/wo this rectified patch.

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 752685 [details] [diff] [review]
Add MParseInt

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

Putting to r-, since there is a major fault in this patch and should never get committed.

Comment 12

[Dave Garrett]

Is it viable to create a corrected patch or is this bug obsolete?

Comment 13

[Hannes Verschore [:h4writer]]

(In reply to Dave Garrett from comment #12)
> Is it viable to create a corrected patch or is this bug obsolete?

I can create a corrected patch out of this one quite easily, but as far as I know it doesn't give a speed-up.
Is there maybe another testcase you are looking at that would have a nice boost with this? The only reason I'm not committing this, is because it possible doesn't gain much for the added complexity/code...

Comment 14

[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.