1080991 - Assertion failure: op->block()->dominates(resume->block()) (Resume point is not dominated by its operands), at jit/IonAnalysis.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

(function(){
    for (var i = 0; i < 8; i++) {
        var c = i
    }
})()

asserts js debug shell on m-c changeset 95d1486223f7 with --ion-eager --ion-loop-unrolling=on --no-threads at Assertion failure: op->block()->dominates(resume->block()) (Resume point is not dominated by its operands), at jit/IonAnalysis.cpp.

Debug configure options:

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-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/628fabf04fda
user:        Dan Gohman
date:        Wed Oct 08 18:21:46 2014 -0700
summary:     Bug 1070258 - IonMonkey: Assert that resume points are dominated by their operands r=nbp

Dan, is bug 1070258 a possible regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0xcaf2e, 0x00000001002b34af js-dbg-opt-64-dm-nsprBuild-darwin-95d1486223f7`js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&) [inlined] AssertDominatorTree(graph=<unavailable>) + 13 at IonAnalysis.cpp:1948, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001002b34af js-dbg-opt-64-dm-nsprBuild-darwin-95d1486223f7`js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&) [inlined] AssertDominatorTree(graph=<unavailable>) + 13 at IonAnalysis.cpp:1948
    frame #1: 0x00000001002b34a2 js-dbg-opt-64-dm-nsprBuild-darwin-95d1486223f7`js::jit::AssertExtendedGraphCoherency(graph=<unavailable>) + 3250 at IonAnalysis.cpp:2008
    frame #2: 0x0000000100277278 js-dbg-opt-64-dm-nsprBuild-darwin-95d1486223f7`js::jit::OptimizeMIR(mir=0x00000001028b5e40) + 2280 at Ion.cpp:1642
    frame #3: 0x0000000100279a47 js-dbg-opt-64-dm-nsprBuild-darwin-95d1486223f7`js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool, js::ExecutionMode) [inlined] js::jit::CompileBackEnd(mir=0x00000001028b5fa8, aRhs=<unavailable>) + 42 at Ion.cpp:1836
    frame #4: 0x0000000100279a1d js-dbg-opt-64-dm-nsprBuild-darwin-95d1486223f7`js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool, js::ExecutionMode) [inlined] js::jit::IonCompile(script=<unavailable>, baselineFrame=<unavailable>, executionMode=SequentialExecution) + 758 at Ion.cpp:2126
(lldb)

Comment 2

[Dan Gohman [:sunfish]]

Attachment: ignore-discarded-resumepoint-operands.patch

Another instance of bug 1055690. Discarded operands are getting left in resume points. I left a note in bug 1055690 mentioning this bug, and included a comment in this bug, so when the underlying bug gets fixed, we can re-enable the assert.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Replace unused Phis by an optimized-out constant.

This patch should fix the current issue without removing the failing
assertion.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8503317 [details] [diff] [review]
ignore-discarded-resumepoint-operands.patch

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

I think this is better if we can remove such corner cases and start having stringer invariants,

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> I think this is better if we can remove such corner cases and start having
> stringer invariants,

s/stringer/stronger/

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8504191 [details] [diff] [review]
Replace unused Phis by an optimized-out constant.

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

Cool! Yes, I agree this is a better way. Can this make the isDiscarded() check in releaseResumePointOperands unnecessary too?

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #6)
> Cool! Yes, I agree this is a better way. Can this make the isDiscarded()
> check in releaseResumePointOperands unnecessary too?

So far this seems to be the case, so I turned the condition into an assertion.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> (In reply to Dan Gohman [:sunfish] from comment #6)
> > Cool! Yes, I agree this is a better way. Can this make the isDiscarded()
> > check in releaseResumePointOperands unnecessary too?
> 
> So far this seems to be the case, so I turned the condition into an
> assertion.

Correction, this does not seems to work with parallel test cases yet. So I will investigate that later and land the current patch.

Comment 9

[Nicolas B. Pierron [:nbp]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a88135fe6b25

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5bc73600d2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d5bc73600d2