Closed Bug 677066 Opened 8 years ago Closed 8 years ago

IonMonkey: Emitting instructions at uses during lowering is incorrect in some cases

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: dvander)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

A bunch of issues have come up as a result of emitAtUses.

1) Given a control flow graph:

   /->3-\
 1-      ->4
   \->2-/

A constant / box defined in block 1, used in block 2, and used as a phi input in block 4 (from the block 3 edge) can get emitted at use in block 2 and not in block 1 or 3. As a result, the phi input is bogus on that edge.

In this case (see attached), the greedy allocator produces a program that returns a bogus value, and the linear scan allocator trips the assertion added in bug 677041 due to an escaping live variable.

2) Emit at uses changes the ID of the original instruction at every emission. Because phis are lowered after all other instructions, phis get the most recent ID, which does not necessarily dominate the appropriate predecessor of the phi.

I have a test case for this one lying around somewhere, when I find it I'll attach it.
Assignee: general → dvander
Status: NEW → ASSIGNED
Duplicate of this bug: 677314
Duplicate of this bug: 676721
Attached patch fix (obsolete) — Splinter Review
Andrew and I both agreed the problem here is how phis are lowered. In this patch:
 * All LBlocks and LPhis are created up-front, before any lowering.
 * Phis are defined when we reach the top of a block, but not lowered.
 * Inputs to phis are lowered individually at their incoming edges.
Attachment #551675 - Flags: review?(adrake)
Attached file Test case 2
Fix regresses this test case. B3 looks particularly suspicious:

10 phi ([t:10]) (v7:*), (v7:*) <|@
11 phi ([d:11]) (v7:*), (v4:*) <|@
12 return () (v10:ecx), (v11:edx) <|@
Attached patch fixSplinter Review
Thanks, there was a stupid typo, op->type() instead of op->id()
Attachment #551675 - Attachment is obsolete: true
Attachment #551675 - Flags: review?(adrake)
Attachment #551750 - Flags: review?(adrake)
Comment on attachment 551750 [details] [diff] [review]
fix

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

Looks good, and passes all the test cases I've got!
Attachment #551750 - Flags: review?(adrake) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/a82301dbc678
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Tried "testcase 2" (x64) on tip and on the pushed revision. On both I get the error: 

ra82301dbc678:
Assertion failure: live->empty(), at /home/h4writer/Build/ionmonkey/js/src/ion/LinearScan.cpp:475
Aborted

r29bd198d1ad1:
Assertion failure: live->empty(), at /home/h4writer/Build/ionmonkey/js/src/ion/LinearScan.cpp:514
Aborted

(This error was marked a duplicate of this bug, so that's why I'm reporting this in this bug)
Filed new bug for that bug (#678072).
Note: it is another testcase and not "testcase 2" I mentioned in the previous comment.
You need to log in before you can comment on or make changes to this bug.