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?

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

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.

(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

You should have first requested sec-approval...

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?

No sec approval needed for PJS.

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

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

JSBugMon: This bug has been automatically verified fixed.

JSBugMon: This bug has been automatically verified fixed on Fx32