865259 - PJS: numerous jit_test failures on tip

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

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.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Specialize arith ops based on baseline feedback

Comment 2

[Niko Matsakis [:nmatsakis]]

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.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: 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.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Part 2: Give MLambda and MParLambda resultTypeSets

Fixes closure-3 (maybe others?)

Comment 5

[Brian Hackett [Laid off!]]

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.

Comment 6

[Niko Matsakis [:nmatsakis]]

Try run for my patch: https://tbpl.mozilla.org/?tree=Try&rev=053e24ed1d31

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Part 2: Give MLambda and MParLambda resultTypeSets

Applying the good catches from bhackett.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Part 2: Give MLambda and MParLambda resultTypeSets v2

Argh, forgot to qref.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: 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.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: 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?

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: 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.

Comment 12

[Shu-yu Guo [:shu]]

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?

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Make comparisons of numerics work regardless of inferred types

Carrying over r+ from bhacket, incorporating f? from Shu

Comment 14

[Niko Matsakis [:nmatsakis]]

Attachment: Make comparisons of numerics work regardless of inferred types

Carrying over r+ from bhackett

Comment 15

[Niko Matsakis [:nmatsakis]]

Attachment: Make comparisons of numerics work regardless of inferred types

Fix windows link error by avoiding being clever with templates. Carrying over r+ from bhackett.

Comment 16

[Niko Matsakis [:nmatsakis]]

Patches pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e463630560f7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/adef5d952d72
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1007963b1ca
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/171ed33b6622

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=e62ead8bd882

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e463630560f7
https://hg.mozilla.org/mozilla-central/rev/adef5d952d72
https://hg.mozilla.org/mozilla-central/rev/b1007963b1ca
https://hg.mozilla.org/mozilla-central/rev/171ed33b6622