702009 - IonMonkey: Add a Movable flag

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

LICM and GVN are hoisting and eliminating MLoadSlot instructions. This is incorrect; an MStoreSlot instruction can write to the same slot in the meantime (or another function could change the value, etc.)

Noticed this when looking at our code for bitwise-and.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Fix

Without the patch, the test fails with --ion -n.

Comment 2

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

LoadSlot is idempotent, since it's not effectful. However it's not necessarily movable because it may have an implicit dependency on a side-effecting instruction. For now, it should be okay to introduce (or re-use) a separate "movable" flag, but long-term we should improve GVN to have simple alias sets like nanojit.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to David Anderson [:dvander] from comment #2)
> LoadSlot is idempotent, since it's not effectful. However it's not
> necessarily movable because it may have an implicit dependency on a
> side-effecting instruction. 

Ah, that's what I thought initially; then I looked at the callers of isIdempotent() and assumed it implied "movable" in Ion. A separate movable flag makes sense, I will add it next week.

> long-term we should improve GVN to have simple alias sets like nanojit.

I don't know how this works in nanojit, but if it does what I think it does we should also try to use it for LICM. For this loop:

for (var i=0; i<a.length; i++)
    x += a[i];

We should be able to reason that no instruction in the loop body "can modify an array length", so it's safe to hoist a.length. Maybe a separate "effect analysis" pass/algorithm which we can use for both LICM and GVN or something.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Add Movable flag

The NeverHoisted flag was never set, so I replaced it with Movable. I considered using NeverMove, but I think Movable is a bit safer (you have to set it explicitly) and it seems more in line with Idempotent.

I added Movable to all Idempotent instructions except MSlots and MLoadSlot. It's not safe to move MSlots because slots can be reallocated. JM uses TI's WatchObjectStateChange for the global's slots to trigger recompilation if the global slots are reallocated. We should use it once we have on stack invalidation.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 574247 [details] [diff] [review]
Add Movable flag

Obsoleted by alias sets.