1007027 - Assertion failure: it.canOptimizeOutIfUnused(), at jit/shared/Lowering-shared.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

(function(x) {
    x = i ? 4 : 2
    y
})()

asserts js debug shell on m-i changeset 23d16c2f0f67 with --ion-eager --ion-parallel-compile=off at Assertion failure: it.canOptimizeOutIfUnused(), at jit/shared/Lowering-shared.cpp

My configure flags are:

AR=ar sh /home/fuzz2lin/trees/mozilla-inbound/js/src/configure --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   181896:87d402b80185
user:        Shu-yu Guo
date:        Tue May 06 19:20:47 2014 -0700
summary:     Bug 1005458 - Argument slot phis are always observable in non-strict scripts due to Function.arguments. (r=nbp)

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to compile specified revision 23d16c2f0f67 (maybe try another?)

Comment 2

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

(In reply to Christian Holler (:decoder) from comment #1)
> JSBugMon: Cannot process bug: Error: Failed to compile specified revision
> 23d16c2f0f67 (maybe try another?)

Let's get JSBugMon to retry later today after the m-i to m-c merge happens.

Comment 3

[Shu-yu Guo [:shu]]

Nicolas and I discussed this bug briefly on IRC. To recap, this bug is because of the buggy way MPhi tracks its own frame slot. When an MPhi gets created, its slot_ member gets the stack position at that point, which is 4. This slot_ member doesn't get updated as it gets setarg'd into slot 3, the first argument slot. The observability analysis then thinks the phi is in slot 4, which is *not* an argument slot, and then incorrectly eliminates the phi, thinking it's unobservable. Here's a disassembly to help see.

00000:  getgname "i"
00005:  ifeq 17 (+12)
00010:  int8 4
00012:  goto 19 (+7)
00017:  int8 2         <- MPhi of the ternary operator created here, with slot_ = 4
00019:  setarg 0       <- This moves the MPhi into slot 3, but the phi's slot_ is still == 4
00022:  pop
00023:  getgname "y"
00028:  pop
00029:  retrval

Nicolas said that he needs to fix this for DCE anyways, and has agreed to take the bug. :)

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: bug1007027-remove-mphi-slot.patch

This patch remove the MPhi::slot field, uses of the slot index are replaced by:
 - In Odin, when iterating over loop-header, we assume that Phis are inserted in the slot order.
 - In Ion, when iterating, we use the ResumePoint operands to read all the Phis.
 - In Ion, when removing Phis, we check the isObserved flag, which is added when a MDefinition is set as operand of a ResumePoint's slot which is observable.

Note, that as soon as one MDefinition appears in one observed slots of one resume point, it would be flagged as Observed.  This ensure that we would always be conservative.

Still this flag is not removed before replacing the operand, otherwise it might become too costly to do so as this would imply checking all resume point uses each time we remove one instance and this might make some optimization to become quadratic (such as scalar replacement, in its current implementation) in the number of resume point which are capturing an MDefinition.  If this becomes a performance issue, we can later add a phase to reset the Observed flags.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8418947 [details] [diff] [review]
bug1007027-remove-mphi-slot.patch

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

The one concern I have, is that you now only look at the phi's in resumepoints to get merged in loopheaders. While we want other (not captured in resumepoints) phis to be added. So those won't get the same threatment of getting correct and added etc!

::: js/src/jit/IonAnalysis.cpp
@@ +112,1 @@
>                  continue;

Since you have two different comments for the two different checks, I think you should not merge them. So just put them on different lines, like: 

> // comment
> if (ins->isImplicitlyUsed())
>     continue;
>  
> // comment
> if (ins->isObserved())
>     continue;

::: js/src/jit/IonBuilder.cpp
@@ +447,5 @@
> +            MResumePoint *oldEntryRp = oldEntry->entryResumePoint();
> +            size_t stackDepth = oldEntryRp->numOperands();
> +            for (size_t slot = 0; slot < stackDepth; slot++) {
> +                MPhi *oldPhi = oldEntryRp->getOperand(slot)->toPhi();
> +                MPhi *newPhi = entry->getSlot(slot)->toPhi();

You want eventually being able to add phi's that aren't captured on the stack, right. But we still want to analyze those phi's! So I think we need to keep the old behaviour here.

::: js/src/jit/MIRGraph.cpp
@@ +1192,5 @@
> +    // We must be a pending loop header
> +    MOZ_ASSERT(kind_ == PENDING_LOOP_HEADER);
> +
> +    size_t stackDepth = entryResumePoint()->numOperands();
> +    for (size_t slot = 0; slot < stackDepth; slot++) {

Same here. We want to add the phis that aren't captured in resumePoints too. Since we will lateron add phi's that don't have a stack position, but still want them be correct used in blocks...

::: js/src/jit/MIRGraph.h
@@ +207,5 @@
>      // Propagates phis placed in a loop header down to this successor block.
>      void inheritPhis(MBasicBlock *header);
>  
> +    // Propagates backedge slots into phis operands of the loop header.
> +    bool inheritPhisFromBackedge(MBasicBlock *backedge, bool *hadTypeChange);

I see no reason to make this public. Are you considering to use this in another patch maybe?

Comment 6

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> The one concern I have, is that you now only look at the phi's in
> resumepoints to get merged in loopheaders. While we want other (not captured
> in resumepoints) phis to be added. So those won't get the same threatment of
> getting correct and added etc!

There is no such things as Phi which are not listed in resume points.  Otherwise this would mean that we are emulating the interpreter stack without restoring it.

> ::: js/src/jit/IonAnalysis.cpp
> @@ +112,1 @@
> >                  continue;
> 
> Since you have two different comments for the two different checks, I think
> you should not merge them. So just put them on different lines, like: 
> 
> > // comment
> > if (ins->isImplicitlyUsed())
> >     continue;
> >  
> > // comment
> > if (ins->isObserved())
> >     continue;
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +447,5 @@
> > +            MResumePoint *oldEntryRp = oldEntry->entryResumePoint();
> > +            size_t stackDepth = oldEntryRp->numOperands();
> > +            for (size_t slot = 0; slot < stackDepth; slot++) {
> > +                MPhi *oldPhi = oldEntryRp->getOperand(slot)->toPhi();
> > +                MPhi *newPhi = entry->getSlot(slot)->toPhi();
> 
> You want eventually being able to add phi's that aren't captured on the
> stack, right. But we still want to analyze those phi's! So I think we need
> to keep the old behaviour here.

No, Not at this stage.

> ::: js/src/jit/MIRGraph.cpp
> @@ +1192,5 @@
> > +    // We must be a pending loop header
> > +    MOZ_ASSERT(kind_ == PENDING_LOOP_HEADER);
> > +
> > +    size_t stackDepth = entryResumePoint()->numOperands();
> > +    for (size_t slot = 0; slot < stackDepth; slot++) {
> 
> Same here. We want to add the phis that aren't captured in resumePoints too.
> Since we will lateron add phi's that don't have a stack position, but still
> want them be correct used in blocks...

Yes, we want to add phi later,
No, we do not care here because this is only used for the creation of basic blocks.

> ::: js/src/jit/MIRGraph.h
> @@ +207,5 @@
> >      // Propagates phis placed in a loop header down to this successor block.
> >      void inheritPhis(MBasicBlock *header);
> >  
> > +    // Propagates backedge slots into phis operands of the loop header.
> > +    bool inheritPhisFromBackedge(MBasicBlock *backedge, bool *hadTypeChange);
> 
> I see no reason to make this public. Are you considering to use this in
> another patch maybe?

So, I was just considering following the inheritPhis functions which has similar use-cases.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8418947 [details] [diff] [review]
bug1007027-remove-mphi-slot.patch

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

After closer examination and discussion on IRC, it seems like we will have to drop the PhiIterator and indeed loop the stack here instead.
The only remaining nit is the jit/IonAnalysis.cpp. So r+

Comment 9

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/646c20d6d346

Comment 10

[Daniel Holbert [:dholbert]]

Backed out, at nbp's request, for jit test failures.
 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9c1e1cf1c1

Sample test failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=39694654&tree=Mozilla-Inbound

Comment 11

[Nicolas B. Pierron [:nbp]]

I am able to reproduce the failure.
I did checked my patch locally, and did not sent it to try because it should be well covered by the jit-tests.

The problem was that the command that I used to check locally did not include the jit-tests testing with --ion-eager --ion-parallel-compile=off:

  $ python …/jit-test/jit_test.py --no-slow ./js

Which is caused by my wrapper script which was checking that "js/src/ion" directory exists to give the --ion argument to the jit-test script.  ( => it is no longer checking because of the directory renaming from "ion" to "jit" :( )

Comment 12

[Nicolas B. Pierron [:nbp]]

Ok, the problem is that when we are using iterator to build the content of
an array we are making a loop in which we have no phi node because its slot
remains unchanged during the loop.

I added cases where if the operand of the resume point is not a Phi, in
which case I assert that:
 - The instruction is defined in a block which precedes the loop header
 - The backedge slot is equal to the ResumePoint operand.

Note, this only matters for loops and not for the pre-header (in case of
OSR), as in such case all slots are merged with Phi nodes.

Comment 13

[Nicolas B. Pierron [:nbp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=b237bc3c9dc1

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e18fd30243f

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e18fd30243f

Comment 16

[Dan Gohman [:sunfish]]

I committed this follow-up while working on bug 1010586:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a2d22d5eb3

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13a2d22d5eb3