Closed Bug 1090037 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

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?
Flags: needinfo?(sunfish)
Attached file 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)
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.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(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.
Attachment #8513743 - Flags: review?(sunfish) → review+
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;
}
Attachment #8513743 - Flags: review?(hv1989) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/93ddc86a09e9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I assume ff35 is also affected. Can you look to uplift this to aurora?
Blocks: 1064537
Flags: needinfo?(nicolas.b.pierron)
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.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8513743 - Flags: approval-mozilla-aurora?
Attachment #8513743 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: