1133149 - Further testcase for "Assertion failure: phi->hasUses() (Missed an unused phi)"

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Alon Zakai (:azakai)]

Attachment: newfail1_bug_1117882.c

Even after bug 1117882 landed, I see this assert. Attached is a new testcase showing it.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: gvn-guard-phi.patch

Range analysis sometimes sets isGuard on arbitrary instructions. When it does this to a phi, we can't delete it, so disable the assert and the rerun check in that case.

Comment 2

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

(function() {
    'use asm'
    function z() {
        var c = 0
        var d = 0
        var e = 0
        var g = 0
        var a = 0
        var b = 0
        var f = 0
        while (1) {
            a = _() | 0
            b = (a << 16) > -4
            if (b) {} else {
                c = a
                break
            }
        }
        d = (c << 16) == 0
        while (1) {
            if (!(d)) {
                break
            }
            e = (f - 1) | 0
            g = (f | 0) > 0
            if (g) {
                f = e
            } else
                break
        }
    }
})()

asserts js debug shell on m-c rev 81f979b17fbd with --fuzzing-safe --no-threads --ion-eager at Assertion failure: phi->hasUses() (Missed an unused phi), at jit/ValueNumbering.cpp

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150202082836" and the hash "c3714f520752".
The "bad" changeset has the timestamp "20150202091038" and the hash "db24c7441b79".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3714f520752&tochange=db24c7441b79

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #1)
> Range analysis sometimes sets isGuard on arbitrary instructions. When it
> does this to a phi, we can't delete it, so disable the assert and the rerun
> check in that case.

Sounds that this is bogus and that the operands of the Phi nodes should have this guard flag instead of the Phi.  It does not make sense to flag an MPhi as a guard as it-self, it does not filter any input.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8564450 [details] [diff] [review]
gvn-guard-phi.patch

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

Could we set the Guard flag on the operands instead of setting it on the MPhi?

Comment 5

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Could we set the Guard flag on the operands instead of setting it on the
> MPhi?

What if the operands are themselves phis?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > Could we set the Guard flag on the operands instead of setting it on the
> > MPhi?
> 
> What if the operands are themselves phis?

Then we should push these flags to their operands, and avoid loops.

Another way would be to push the Guard flag to the operands when we are removing unused Phi.

Comment 7

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 993eb76a8bd6).

Comment 8

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

Not sure what's next here.

Comment 9

[Dan Gohman [:sunfish]]

This appears to be fixed in 31d7b208abd1 (bug 1130679). Range analysis is no longer setting the isGuard flag in the same way.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Dan Gohman [:sunfish] from comment #9)
> This appears to be fixed in 31d7b208abd1 (bug 1130679). Range analysis is no
> longer setting the isGuard flag in the same way.

Closing then :)