PJS: numerous jit_test failures on tip

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

unspecified
mozilla23
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

7.89 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
1.44 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.47 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
20.46 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
When running jit-test.py, I see the following test failures on tip:

FAILURES:
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/bailout-executed.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/bug853576.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/bug854021.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/closure-3.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/comprehension-2.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/comprehension-fn-args.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/element-2.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/filter-non-divisible.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/filter-every-third-element.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/filter-all.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/filter-very-few.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/flatten-1.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/filter-none.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/filter-truthy.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/flatten-2.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/get-2.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/length-2.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/map-3.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/map-inc.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/reduce-higher-dim.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/scan-3.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/scan-1.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/scatter-6.js
    /home/nmatsakis/versioned/iontrail-2/js/src/jit-test/tests/parallelarray/shape-2.js

According to bisect, these failures were introduced by bug 804676 (commit <https://hg.mozilla.org/mozilla-central/rev/ee14945b452c>, to be precise). They appear to all be due to mathematical operations that used to be specialized but now are not. I believe that these failures due not occur on TBPL due to bug 856246.

Conversation with bhackett on IRC suggests that what is needed is to modify the specialization code to consider baseline type information.
(Assignee)

Comment 1

5 years ago
Created attachment 741455 [details] [diff] [review]
Specialize arith ops based on baseline feedback
Attachment #741455 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

5 years ago
Comment on attachment 741455 [details] [diff] [review]
Specialize arith ops based on baseline feedback

Removing request for review because I realized this only solves some of the test failures, not all.
Attachment #741455 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
Created attachment 741619 [details] [diff] [review]
Specialize arith ops based on baseline feedback

This patch specializes the arith ops based on baseline feedback rather than letting them remain unspecialized. For bonus points, I also cause us to specialize to double as a last resort in par exec mode---this conforms to our intended subset of supporting binary ops on numbers but not objects, without having to implement any ICs or anything like that.

This patch does not fix all test failures. I still see errors in bailout-executed, closure-3, and filter-very-few. closure-3 is being investigated by shu, not sure what's going on with the other two.
Attachment #741455 - Attachment is obsolete: true
Attachment #741619 - Flags: review?(bhackett1024)

Comment 4

5 years ago
Created attachment 741620 [details] [diff] [review]
Part 2: Give MLambda and MParLambda resultTypeSets

Fixes closure-3 (maybe others?)
Attachment #741620 - Flags: review?(bhackett1024)
Attachment #741619 - Flags: review?(bhackett1024) → review+
Comment on attachment 741620 [details] [diff] [review]
Part 2: Give MLambda and MParLambda resultTypeSets

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

::: js/src/ion/MIR.h
@@ +3857,5 @@
>      MLambda(MDefinition *scopeChain, JSFunction *fun)
>        : MUnaryInstruction(scopeChain), fun_(fun)
>      {
>          setResultType(MIRType_Object);
> +        setResultTypeSet(MakeSingletonTypeSet(fun));

Per LIRGenerator::visitLambda, if UseNewTypeForClone holds for the function then the result will not have the same type as fun.  You can just leave the default NULL result type set in this case.
Attachment #741620 - Flags: review?(bhackett1024) → review+

Comment 7

5 years ago
Created attachment 741627 [details] [diff] [review]
Part 2: Give MLambda and MParLambda resultTypeSets

Applying the good catches from bhackett.
Attachment #741620 - Attachment is obsolete: true
Attachment #741627 - Flags: review+

Comment 8

5 years ago
Created attachment 741629 [details] [diff] [review]
Part 2: Give MLambda and MParLambda resultTypeSets v2

Argh, forgot to qref.
Attachment #741627 - Attachment is obsolete: true
Attachment #741629 - Flags: review+

Comment 9

5 years ago
Created attachment 741729 [details] [diff] [review]
Part 3: Specialize int32 compares from baseline IC stub info

Just like for doubles.

This is sufficient for parallelarray/filter-very-few.js to pass.
Attachment #741729 - Flags: review?(bhackett1024)

Comment 10

5 years ago
Created attachment 741732 [details] [diff] [review]
Quick & dirty proof of concept to reinfer uses of eliminated phis

The parallelarray/bailout-executed.js test is failing because we are not reinferring types on MIR that depend on eliminated phis (especially due to unreachable code elimination).

This is a very rough proof of concept to make the test pass. The idea is to reinfer |MCompare|s that had a data dependency on eliminated phis. The plan is to make |infer()| remember its arguments on a MIR-by-MIR basis, and |reinfer()| a nullary virtual on |MDefinition| that 1) resets type-related state on the MIR, like specializations and undoing what its type policy did; 2) passes in the saved args from the first call to |infer()| back into itself.

Brian, do you see a cleaner way to do this?
Attachment #741732 - Flags: feedback?(bhackett1024)
Attachment #741729 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 741806 [details] [diff] [review]
Make comparisons of numerics work regardless of inferred types

As an alternative / complement to attachment 741729 [details] [diff] [review] and attachment 741732 [details] [diff] [review], this patch makes comparisons work even on boxed values. Attachment 741729 [details] [diff] is probably still valuable, but I think attachment 741732 [details] [diff] [review] can wait (though I think it would be a good optimization as well).

With this patch + the other two, all tests pass for me.
Attachment #741806 - Flags: review?(bhackett1024)
Attachment #741806 - Flags: feedback?(shu)
Attachment #741806 - Flags: review?(bhackett1024) → review+

Comment 12

5 years ago
Comment on attachment 741806 [details] [diff] [review]
Make comparisons of numerics work regardless of inferred types

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

Cool, now we can at least not bail out for comparing values that don't require effectful coercions. I'll move the re-inference patch to a different bug.

::: js/src/ion/CodeGenerator.cpp
@@ +3250,5 @@
> +
> +    switch (lir->mir()->jsop()) {
> +      case JSOP_EQ:
> +      case JSOP_STRICTEQ:
> +        return callVM(ParEqInfo, lir);

Calling the same function for both EQ and STRICTEQ isn't right. Need to ensure the operands have the same type for strict equality.

@@ +3254,5 @@
> +        return callVM(ParEqInfo, lir);
> +
> +      case JSOP_NE:
> +      case JSOP_STRICTNE:
> +        return callVM(ParNeInfo, lir);

Ditto.

::: js/src/ion/MIR.h
@@ +1872,5 @@
> +// because we require access to the ForkJoinSlice.
> +//
> +// Currently, all parallel comparisons are SIDE-EFFECT FREE, because
> +// we only do not handle comparisons that require coercion of objects.
> +class MParCompareValues

Nit: for consistency's sake, maybe rename this to MParCompareVM?

@@ +1899,5 @@
> +    }
> +
> +    MDefinition *parSlice() const {
> +        return getOperand(0);
> +    }

callVM gets the current slice from TLS. What's this used for?
Attachment #741806 - Flags: feedback?(shu)
(Assignee)

Comment 13

5 years ago
Created attachment 742115 [details] [diff] [review]
Make comparisons of numerics work regardless of inferred types

Carrying over r+ from bhacket, incorporating f? from Shu
Assignee: general → nmatsakis
Attachment #741732 - Attachment is obsolete: true
Attachment #741806 - Attachment is obsolete: true
Attachment #741732 - Flags: feedback?(bhackett1024)
Attachment #742115 - Flags: review+
(Assignee)

Comment 14

5 years ago
Created attachment 742321 [details] [diff] [review]
Make comparisons of numerics work regardless of inferred types

Carrying over r+ from bhackett
Attachment #742115 - Attachment is obsolete: true
Attachment #742321 - Flags: review+
(Assignee)

Comment 15

5 years ago
Created attachment 742524 [details] [diff] [review]
Make comparisons of numerics work regardless of inferred types

Fix windows link error by avoiding being clever with templates. Carrying over r+ from bhackett.
Attachment #742321 - Attachment is obsolete: true
Attachment #742524 - Flags: review+
You need to log in before you can comment on or make changes to this bug.