680621 - IonMonkey: Snapshot captures phi of successor block

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Drake [:adrake]]

Attachment: Test case

Attached test case produces a graph with a snapshot capturing a phi from a successor block on ionmonkey tip x86 debug builds.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: instrumentation

Comment 3

[Andrew Drake [:adrake]]

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

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.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Additionally there was another (potential) bug. We cannot make the test-compare optimization if the compare is not idempotent.

Comment 6

[Sean Stangl [:sstangl]]

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.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/projects/ionmonkey/rev/cc659b5d13af