Closed Bug 724975 Opened 12 years ago Closed 12 years ago

IonMonkey: Assertion failure: loop_->loopHeader() == block->loopHeaderOfBackedge(), at ion/AliasAnalysis.cpp:165

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: jandem)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

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;
}
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.
(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.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
We weren't compiling labeled breaks since I added JSOP_LABEL (bug 684384).
Attachment #596664 - Flags: review?(dvander)
Attachment #596664 - Flags: review?(dvander) → review+
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.
Attachment #596971 - Flags: review?(dvander)
Fix an off-by-one error.
Attachment #596971 - Attachment is obsolete: true
Attachment #596971 - Flags: review?(dvander)
Attachment #596992 - Flags: review?(dvander)
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.
Attachment #596993 - Flags: review?(dvander)
Attachment #596993 - Attachment is patch: true
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?
Attachment #596992 - Flags: review?(dvander) → review+
Attachment #596993 - Flags: review?(dvander) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug724975.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.