Last Comment Bug 724975 - IonMonkey: Assertion failure: loop_->loopHeader() == block->loopHeaderOfBackedge(), at ion/AliasAnalysis.cpp:165
: IonMonkey: Assertion failure: loop_->loopHeader() == block->loopHeaderOfBacke...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-02-07 10:00 PST by Christian Holler (:decoder)
Modified: 2013-01-14 08:03 PST (History)
5 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Compile JSOP_LABEL (623 bytes, patch)
2012-02-13 06:09 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Review
Part 2: Fix some loop depth problems (12.24 KB, patch)
2012-02-14 04:55 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Review
Part 2: Fix some loop depth problems (12.24 KB, patch)
2012-02-14 05:57 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Review
Part 3: Fix ReorderBlocks (2.97 KB, patch)
2012-02-14 06:07 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-02-07 10:00:56 PST
The following testcase asserts on ionmonkey revision c34398f961e7 (run with --ion -n), tested on 64 bit:


var SECTION = "15.5.4.7-2";
(SECTION, "var s = new String('hello'); s.lastIndexOf('ll', 4)",
("hello", "ll", 4), "var s = new String('hello'); s.lastIndexOf('ll', 4)")(SECTION, "var s = new String('hello'); s.lastIndexOf('ll', 5)", LastIndexOf("hello", "ll", 5), "var s = new String('hello'); s.lastIndexOf('ll', 5)")(SECTION, "var s = new String('hello'); s.lastIndexOf('ll', 6)", ("hello", "ll", 6), "var s = new String('hello'); s.lastIndexOf('ll', 6)");
function LastIndexOf(string, search, position) {
    for (;;) for (;;) if (string != search) break;
}
Comment 1 David Anderson [:dvander] 2012-02-10 19:00:01 PST
The bug here appears to be that alias analysis wants to visit inner backedges before outer ones, which is an assumption we also make in the greedy allocator (FindNaturalLoops). I'm not sure whether this is a bad assumption on our part or whether there is an easy fix to block re-ordering.
Comment 2 Jan de Mooij [:jandem] 2012-02-13 05:48:36 PST
(In reply to David Anderson [:dvander] from comment #1)
> The bug here appears to be that alias analysis wants to visit inner
> backedges before outer ones, which is an assumption we also make in the
> greedy allocator (FindNaturalLoops). I'm not sure whether this is a bad
> assumption on our part or whether there is an easy fix to block re-ordering.

I'd like to maintain this invariant; it simplifies loop-based analyses and may avoid some correctness issues. We should try to fix block re-ordering so that loop blocks are contiguous, this will also allow us to remove some code to handle non-contiguous loops from LSRA.

I have a WIP patch that seems to work, but I hit some unrelated problems along the way, going to fix these first.
Comment 3 Jan de Mooij [:jandem] 2012-02-13 06:09:53 PST
Created attachment 596664 [details] [diff] [review]
Part 1: Compile JSOP_LABEL

We weren't compiling labeled breaks since I added JSOP_LABEL (bug 684384).
Comment 4 Jan de Mooij [:jandem] 2012-02-14 04:55:47 PST
Created attachment 596971 [details] [diff] [review]
Part 2: Fix some loop depth problems

Fix the loop depths for blocks from inlined functions, "broken" loops and some other cases. An alternative is to add a pass to calculate loop depths (after critical edge splitting), I can do that in a follow-up bug if you want.
Comment 5 Jan de Mooij [:jandem] 2012-02-14 05:57:12 PST
Created attachment 596992 [details] [diff] [review]
Part 2: Fix some loop depth problems

Fix an off-by-one error.
Comment 6 Jan de Mooij [:jandem] 2012-02-14 06:07:02 PST
Created attachment 596993 [details] [diff] [review]
Part 3: Fix ReorderBlocks

Changes ReorderBlocks to not insert any loop successor blocks between the loop header and backedge. These blocks are added to a vector and inserted after the backedge.
Comment 7 David Anderson [:dvander] 2012-02-14 11:45:01 PST
Comment on attachment 596992 [details] [diff] [review]
Part 2: Fix some loop depth problems

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

::: js/src/ion/IonBuilder.cpp
@@ +1003,5 @@
> +    JS_ASSERT(loopDepth_);
> +    loopDepth_--;
> +
> +    // A broken loop is not a real loop (it has no header or backedge), so
> +    // reset the loop depth.

What's the danger if we just lied about the loop depth for these blocks?
Comment 8 Jan de Mooij [:jandem] 2012-02-16 11:50:02 PST
https://hg.mozilla.org/projects/ionmonkey/rev/4f9b25f60991
https://hg.mozilla.org/projects/ionmonkey/rev/435100cc71d1
https://hg.mozilla.org/projects/ionmonkey/rev/a1500e708273

(In reply to David Anderson [:dvander] from comment #7)
> Comment on attachment 596992 [details] [diff] [review]
> Part 2: Fix some loop depth problems
> 
> What's the danger if we just lied about the loop depth for these blocks?

IIRC, there was no real danger but it broke some invariants I wanted to assert.
Comment 9 Christian Holler (:decoder) 2013-01-14 08:03:39 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug724975.js.

Note You need to log in before you can comment on or make changes to this bug.