1014973 - Assertion failure: ins->input()->type() == MIRType_Double, at jit/Lowering.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Array.buildPar(6763, function() {
    return Math.fround(Math.round(Math.fround(-0)));
});

asserts js debug shell on m-c changeset b40296602083 with --ion-eager --ion-parallel-compile=off at Assertion failure: ins->input()->type() == MIRType_Double, at jit/Lowering.cpp

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/a1b396e1f1dd
user:        Benjamin Bouvier
date:        Fri Feb 28 12:07:05 2014 +0100
summary:     Bug 930477: Specialize Round for Float32; r=jandem,mjrosenb

Setting s-s and sec-critical by default because this seems to be a MIR issue.

Benjamin, is bug 930477 a likely regressor?

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Retry to specialize replaced instructions for float32 in ParallelSafetyAnalysis; NO REVIEW YET

Nice find! ParallelSafetyAnalysis replaces the MMathFunction by another one. The former one has been specialized for
float32 so its arguments are both float32, while the new one hasn't the specialization. One way to solve this would be
to have the MMathFunction sets its type as the type of its input, but the TypeAnalyzer Float32 algorithm assumes that
instructions aren't specialized by default.

Retrying to specialize in visitMathFunction would be rather unfortunate, as we'd need to do that for every instruction
that is Float32 commutative and gets a CUSTOM_OP. Retrying to specialize in replace() is more generic and should prevent
similar issues to show up again in the future.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8427701 [details] [diff] [review]
Retry to specialize replaced instructions for float32 in ParallelSafetyAnalysis; NO REVIEW YET

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

Thanks for fixing this!

The only reason we replace MMathFunction is to stop it from using the cache, which isn't threadsafe. I'm actually not sure off the top of my head why we don't just null out the cache field in the existing MMathFunction...

::: js/src/jit-test/tests/parallel/bug1014973.js
@@ +1,3 @@
> +Array.buildPar(6763, function() {
> +    return Math.fround(Math.round(Math.fround(-0)));
> +});

All PJS tests need to be protected with |if (getBuildConfiguration().parallelJS)|

::: js/src/jit/ParallelSafetyAnalysis.cpp
@@ +648,5 @@
>      block->discard(oldInstruction);
> +
> +    // We may have replaced a specialized Float32 instruction by its
> +    // non-specialized version, so just retry to specialize it. This relies on
> +    // the fact that Phis' types don't change during the ParallelSafetyAnalysis;

FWIW the parallel safety analysis should never mess with type info.

Comment 3

[Shu-yu Guo [:shu]]

I would actually like stronger asserts about the MIR graph pre and post ParallelSafetyAnalysis, namely  that all specializations are preserved. Trouble is, specialization information tends to ad hoc on a MIR-by-MIR basis.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Shu-yu Guo [:shu] from comment #3)
> I would actually like stronger asserts about the MIR graph pre and post
> ParallelSafetyAnalysis, namely  that all specializations are preserved.
> Trouble is, specialization information tends to ad hoc on a MIR-by-MIR basis.
Thanks! I actually added a JS_ASSERT at the end of replace, that the replaced and replacing instructions have the same type (this test case would have triggered it).

https://hg.mozilla.org/integration/mozilla-inbound/rev/548638e8243d

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

You should have first requested sec-approval...

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

As said on irc: it's specific to ParallelJS, which is, as far as i know, only enabled on nightlies. Should i still ask for sec-approval now?

Comment 7

[Shu-yu Guo [:shu]]

No sec approval needed for PJS.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> As said on irc: it's specific to ParallelJS, which is, as far as i know,
> only enabled on nightlies. Should i still ask for sec-approval now?

Ah, ok, thanks for the clarification that this is nightly-only. Adjusting flags.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This landed:

http://hg.mozilla.org/mozilla-central/rev/548638e8243d

Comment 10

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx32