1090037 - 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 f(x) {
    Math.sin([] | 0 && x)
}
for (var j = 0; j < 1; j++) {
    f(1 && 0)
}

asserts js debug shell on m-c changeset 20408ad61ce5 with --ion-eager --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 = 0x4af20, 0x00000001002aff9f js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&) [inlined] AssertDominatorTree(graph=<unavailable>) + 13 at IonAnalysis.cpp:1947, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001002aff9f js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&) [inlined] AssertDominatorTree(graph=<unavailable>) + 13 at IonAnalysis.cpp:1947
    frame #1: 0x00000001002aff92 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::AssertExtendedGraphCoherency(graph=<unavailable>) + 3250 at IonAnalysis.cpp:2007
    frame #2: 0x00000001002ba7e1 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::AccountForCFGChanges(mir=<unavailable>, graph=0x0000000103095c40, updateAliasAnalysis=<unavailable>) + 129 at IonAnalysis.cpp:1488
    frame #3: 0x00000001004270ce js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::ValueNumberer::run(this=0x00007fff5fbfea38, updateAliasAnalysis=<unavailable>) + 750 at ValueNumbering.cpp:1084
    frame #4: 0x00000001002abaa5 js-dbg-opt-64-dm-nsprBuild-darwin-20408ad61ce5`js::jit::OptimizeMIR(mir=0x0000000103095e58) + 1365 at Ion.cpp:1551
(lldb)

Comment 2

[Nicolas B. Pierron [:nbp]]

The problem here, is coming from the fact that MPhi::foldsTernary is calling moving the constant to the immediate dominator, but the dominator computation has not been refreshed before the call to foldsTo.

the Generated code look something like:

  x = 1;
  if (x)
    y = 0;
  y ? y : 0

The problem is that GVN is apparently capable to fold the 2 zeros, but MPhi::foldsTernary is using the dominator to see if the trueDef dominates both branches.  Unfortunately the dominator info is not yet updated and the fold move the constant about the ternary operator.  Which then cause this assertion.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure that dominators are defined enough before moving instructions.

Comment 4

[Dan Gohman [:sunfish]]

Comment on attachment 8513743 [details] [diff] [review]
Ensure that dominators are defined enough before moving instructions.

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

::: js/src/jit/MIR.cpp
@@ +1310,5 @@
> +    // As GVN removed a branch, it will update the dominations rules before
> +    // trying to fold this MPhi again. Thus, this condition does not inhibit
> +    // this optimization.
> +    MBasicBlock *truePred = firstIsTrueBranch ? block()->getPredecessor(0)
> +                                              : block()->getPredecessor(1);

I'd find this expression a little easier to read as block()->getPredecessor(firstIsTrueBranch ? 0 : 1).

::: js/src/jit/ValueNumbering.cpp
@@ +653,5 @@
> +#ifdef DEBUG
> +            JitSpew(JitSpew_GVN,
> +                    "      Recording %s%u",
> +                    def->opName(), def->id());
> +#endif

Please add similar debug output to the overwrite case just above this.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8513743 [details] [diff] [review]
Ensure that dominators are defined enough before moving instructions.

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

Thanks. r+ with the requested changes.

::: js/src/jit/MIR.cpp
@@ +1312,5 @@
> +    // this optimization.
> +    MBasicBlock *truePred = firstIsTrueBranch ? block()->getPredecessor(0)
> +                                              : block()->getPredecessor(1);
> +    if (trueDef == c && !trueDef->block()->dominates(truePred))
> +        return nullptr;

The "tautology" part is only about "trueDef->block()->dominates(truePred)" IIUC.
It doesn't make a difference if "trueDef == c" or not. (Only the fact that we currently deduced it will only happen in that case).

Considering that, can't we test this unconditionally? (With the same explanation).

if (!trueDef->block()->dominates(truePred) ||
    !falseDef->block()->dominates(falsePred))
{
    return nullptr;
}

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> ::: js/src/jit/MIR.cpp
> @@ +1312,5 @@
> > +    // this optimization.
> > +    MBasicBlock *truePred = firstIsTrueBranch ? block()->getPredecessor(0)
> > +                                              : block()->getPredecessor(1);
> > +    if (trueDef == c && !trueDef->block()->dominates(truePred))
> > +        return nullptr;
> 
> The "tautology" part is only about "trueDef->block()->dominates(truePred)"
> IIUC.
> It doesn't make a difference if "trueDef == c" or not. (Only the fact that
> we currently deduced it will only happen in that case).
> 
> Considering that, can't we test this unconditionally? (With the same
> explanation).
> 
> if (!trueDef->block()->dominates(truePred) ||
>     !falseDef->block()->dominates(falsePred))
> {
>     return nullptr;
> }

Yes, we can.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93ddc86a09e9

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93ddc86a09e9

Comment 9

[Hannes Verschore [:h4writer]]

I assume ff35 is also affected. Can you look to uplift this to aurora?

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8513743 [details] [diff] [review]
Ensure that dominators are defined enough before moving instructions.

Thanks Hannes for noticing :)

Approval Request Comment
[Feature/regressing bug #]: Bug 1064537 (the previous patch is just adding a sanity assertion, but the bug is linked to the addition of the optimization)

[User impact if declined]: (see below)

[Describe test coverage new/current, TBPL]: checked on inbound & central.

[Risks and why]: low risk. This patch add additional checks to make sure we have the right context before doing an optimization. The checks is similar to what is already executed in the same function.

[String/UUID change made/needed]: N/A.

Comment 12

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1cdbb76b3199