The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Other Branch
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 2

5 years ago
(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
(Assignee)

Comment 3

5 years ago
Created attachment 596664 [details] [diff] [review]
Part 1: Compile JSOP_LABEL

We weren't compiling labeled breaks since I added JSOP_LABEL (bug 684384).
Attachment #596664 - Flags: review?(dvander)
Attachment #596664 - Flags: review?(dvander) → review+
(Assignee)

Comment 4

5 years ago
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.
Attachment #596971 - Flags: review?(dvander)
(Assignee)

Comment 5

5 years ago
Created attachment 596992 [details] [diff] [review]
Part 2: Fix some loop depth problems

Fix an off-by-one error.
Attachment #596971 - Attachment is obsolete: true
Attachment #596971 - Flags: review?(dvander)
Attachment #596992 - Flags: review?(dvander)
(Assignee)

Comment 6

5 years ago
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.
Attachment #596993 - Flags: review?(dvander)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 8

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

4 years ago
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.