IonMonkey: Snapshot captures phi of successor block

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: adrake, Assigned: dvander)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

8 years ago
Posted file Test case
Attached test case produces a graph with a snapshot capturing a phi from a successor block on ionmonkey tip x86 debug builds.
I can't seem to find where this his happening - and it works on the greedy allocator. For example, here's some spew with the attached patch and IONFLAGS=snapshots:

Note, I did find one bug but it didn't seem to change anything. Note that the only snapshot captured in LIR does not seem to have phis.

Current snapshot 0x8567108 details:
    taken at block 0 entry
    pc: 0x8568c8c
    slot0: parameter2-vn2
    slot1: parameter4-vn4
    slot2: constant6-vn6
    slot3: constant6-vn6
Current snapshot 0x8567108 details:
    taken after:parameter1-vn2
    pc: 0x8568c8c
    slot0: parameter1-vn2
    slot1: parameter4-vn4
    slot2: constant6-vn6
    slot3: constant6-vn6
Current snapshot 0x8567108 details:
    taken after:parameter3-vn4
    pc: 0x8568c8c
    slot0: parameter1-vn2
    slot1: parameter3-vn4
    slot2: constant6-vn6
    slot3: constant6-vn6
Current snapshot 0x8567a08 details:
    taken at block 1 entry
    pc: 0x8568c8c
    slot0: parameter1-vn2
    slot1: parameter3-vn4
    slot2: compare16-vn16
    slot3: constant0-vn6
Current snapshot 0x8567508 details:
    taken at block 2 entry
    pc: 0x8568ca5
    slot0: parameter1-vn2
    slot1: parameter3-vn4
    slot2: compare16-vn16
    slot3: constant0-vn6
[Snapshots] Generating LIR snapshot 0x8568f60 from MIR (0x8567508)
[Snapshots] Assigning snapshot 0x8568f60 to instruction 0x8568f30
Current snapshot 0x8567660 details:
    taken at block 3 entry
    pc: 0x8568cae
    slot0: parameter1-vn2
    slot1: parameter3-vn4
    slot2: compare16-vn16
    slot3: phi28-vn28
[Snapshots] Encoding snapshot 0x8568f60 (nfixed 4) (exprStack 0)
[Snapshots]     slot 0: value (t=144, d=140)
[Snapshots]     slot 1: value (t=152, d=esi)
[Snapshots]     slot 2: boolean (ecx)
[Snapshots]     slot 3: undefined
[Snapshots]     total size: 18 bytes
undefined
Reporter

Comment 3

8 years ago
Here's a trace in gdb showing the bad snapshot:

Assertion failure: live->empty(), at /home/adrake/moz/ionmonkey/js/src/ion/LinearScan.cpp:573

Program received signal SIGABRT, Aborted.
0x00110430 in __kernel_vsyscall ()
(gdb) up 4
#4  0x08394f92 in js::ion::LinearScanAllocator::buildLivenessInfo (this=0xffffbb54) at /home/adrake/moz/ionmonkey/js/src/ion/LinearScan.cpp:573
573	        JS_ASSERT_IF(!mblock->numPredecessors(), live->empty());
(gdb) p live->contains(16)
$1 = true
(gdb) p vregs[16].uses_[1].ins->id()
$2 = 11
(gdb) p vregs[16].uses_[1].ins->snapshot()->getEntry(5)->toUse()->virtualRegister()
$3 = 16

From that dump and this gdb session, it looks like the entry snapshots on the two branches have captured register 16, which does not dominate either branch.
Posted patch fixSplinter Review
Two bugs.

(1) When checking if we can optimize a test(compare) to test-compare, we ignore snapshot uses, so snapshots captured a bogus virtual register. I've rejiggered the API for mapping MIR to virtual registers to assert when this happens in the future. It's super hard to debug otherwise.

(2) For some bizarre reason, during lowering assignSnapshot() looked at the completely wrong snapshot.
Assignee: general → dvander
Attachment #555281 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #555306 - Flags: review?(sstangl)
Additionally there was another (potential) bug. We cannot make the test-compare optimization if the compare is not idempotent.
Comment on attachment 555306 [details] [diff] [review]
fix

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

If you think that the keyword "virtualRegister" used everywhere is unnecessarily verbose, we could instead unambiguously call it a "vreg".

::: js/src/ion/MIR.h
@@ +88,5 @@
>      _(LoopInvariant)                                                        \
>      _(Commutative)                                                          \
>      _(Idempotent) /* The instruction has no side-effects. */                \
> +    _(NeverHoisted) /* Don't hoist, even if loop invariant */               \
> +    _(Lowered) /* has a virtual register */

Comment should express that it is only intended for use in debug mode.
Attachment #555306 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/cc659b5d13af
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.