1016137 - Assertion failure: numOperands() > 1, at jit/MIR.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

Array.buildPar(1, function(x) {
    if (3 == 6) {
        e
        return x
    }
    Math.tan()
})

asserts js debug shell on m-c changeset b5bdc1aaf378 with --ion-eager --ion-parallel-compile=off at Assertion failure: numOperands() > 1, at jit/MIR.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>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140523151622" and the hash "f797fb250394".
The "bad" changeset has the timestamp "20140523151819" and the hash "0ac90fa5f24a".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f797fb250394&tochange=0ac90fa5f24a


s-s because this seems to involve MIR, assuming sec-critical for now.

Dan, is bug 1004363 a likely regressor?

Comment 1

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

Attachment: opt stack

Array.buildPar(8, function(x) {
    if (x % 5 == 2) {
        for (v in y) {}
    }
})

Opt crash [@ js::jit::LiveRangeAllocator]

./js-opt-64-ts-darwin-b5bdc1aaf378 --ion-eager --ion-parallel-compile=off testcase.js
Segmentation fault: 11

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 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe

Comment 2

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

fuzzblocker - :sunfish, please look at this asap?

Comment 3

[Dan Gohman [:sunfish]]

Attachment: reset-succ-with-phis.patch

I haven't been able to reproduce the reported assertion failure. However, I do see a null-pointer dereference elsewhere. The attached patch fixes the null pointer dereference. With this patch, this test now passes for me.

FWIW, the assertion failing in the original report is not important; I already have patches underway which remove it, since it doesn't seem to be protecting any actual assumptions, and since it has a tendancy to fire when it shouldn't.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: precise-successor-with-phis.patch

Actually, here's a better patch. Instead of trying to cope with successorWithPhis being set when there are no phis, just fix the code so that it doesn't set successorWithPhis when there are no phis in the first place. This also re-enables and strengthens a long-standing commented-out assert.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8429659 [details] [diff] [review]
precise-successor-with-phis.patch

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

I do not see how this patch would remove the assertion case reported in comment 0.

I would have changed MBasicBlock::removePredecessor such as when there is only one remaining predecessor, we replace all phis by their operands and remove all Phis from the current block.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: precise-successor-with-phis.patch

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Comment on attachment 8429659 [details] [diff] [review]
> precise-successor-with-phis.patch
> 
> Review of attachment 8429659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do not see how this patch would remove the assertion case reported in
> comment 0.

Sorry for the confusion. That patch fixed a related crash that I saw. I'm now able to reproduce the original assertion failure too, and I've now updated the patch to fix it. The fix is to just remove the assert, since it's not actually guarding any important assumptions.

> I would have changed MBasicBlock::removePredecessor such as when there is
> only one remaining predecessor, we replace all phis by their operands and
> remove all Phis from the current block.

I believe the patch sequence in bug 1004363 is a better way to implement this optimization. By doing this optimization within GVN, we can easily utilize the full power of foldsTo -- a single-operand phi is just a special case of a phi where all operands are the same value, for example.

Comment 7

[Dan Gohman [:sunfish]]

Downgrading to sec-low because the failing assertion can simply be deleted, and because the crash I saw was a simple null pointer dereference.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8430430 [details] [diff] [review]
precise-successor-with-phis.patch

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

::: js/src/jit/MIR.cpp
@@ -919,5 @@
>  void
>  MPhi::removeOperand(size_t index)
>  {
>      JS_ASSERT(index < numOperands());
> -    JS_ASSERT(numOperands() > 1);

I would prefer to keep this assertion for sanity reasons, and to remove all Phi in removePredecessors.

Otherwise we will keep these Phi in the graph, with no special reasons, and this might go against the default assumptions about the MIR Graph.

Comment 9

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 8430430 [details] [diff] [review]
> precise-successor-with-phis.patch
> 
> Review of attachment 8430430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MIR.cpp
> @@ -919,5 @@
> >  void
> >  MPhi::removeOperand(size_t index)
> >  {
> >      JS_ASSERT(index < numOperands());
> > -    JS_ASSERT(numOperands() > 1);
> 
> I would prefer to keep this assertion for sanity reasons, and to remove all
> Phi in removePredecessors.
> 
> Otherwise we will keep these Phi in the graph, with no special reasons, and
> this might go against the default assumptions about the MIR Graph.

I feel strongly about this, so please forgive me making one more appeal :).

Optimizing phis can expose further optimization opportunities. If we replace a phi with a constant value, it may lead to further constant folding and other optimizations. Consequently, we already have a reason to follow up any pass that does removePredecessor with a GVN+UCE pass, to optimize opportunities which get exposed. GVN+UCE do a fine job optimizing single-operand phis, and of course much more. It's cleaner to let removePredecessor just remove a predecessor, instead of also trying to duplicate a small subset of GVN+UCE, given that we'll still want to run the real GVN+UCE after it anyway.

Deleting code is the trickiest part of the new GVN+UCE code I'm writing. It's far trickier than the GVN algorithm itself. It needs to very carefully manage the order in which everything happens to avoid tripping up myriad asserts, and to avoid deleting or modifying something while it's still needed to find other things to delete. If removePredecessor does more than just removing predecessors, keeping track of what order everything happens in could get even trickier. I much prefer to have removePredecessor just remove a predecessor (and minimally keep the IL consistent).

Single-operand phis are valid SSA form. Commonly-used idioms like LCSSA for example make extensive use of single-operand phis (IonMonkey doesn't use LCSSA, but it wouldn't be hard to imagine it doing so in the future). It's surprising to me that this assertion is present in IonMonkey, and it's not surprising to me that I haven't been able to find any code in IonMonkey which actually relies on such an assumption. I don't know of anything that such an assertion would help enable.

So, let's let removePredecessor just remove a predecessor (and keep the IL consistent), and let's let GVN do the GVN (and related optimizations that naturally go with it) afterwards.

Comment 10

[Dan Gohman [:sunfish]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe49160cc5e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfe49160cc5e

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx32