Closed Bug 702009 Opened 10 years ago Closed 10 years ago

IonMonkey: Add a Movable flag

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Fix (obsolete) — Splinter Review
Without the patch, the test fails with --ion -n.
Attachment #574064 - Flags: review?(sstangl)
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.
(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.
Attachment #574064 - Flags: review?(sstangl)
Attached patch Add Movable flagSplinter Review
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.
Attachment #574064 - Attachment is obsolete: true
Attachment #574247 - Flags: review?(sstangl)
Summary: IonMonkey: MLoadSlot should not set the idempotent flag → IonMonkey: Add a Movable flag
Comment on attachment 574247 [details] [diff] [review]
Add Movable flag

Obsoleted by alias sets.
Attachment #574247 - Flags: review?(sstangl)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.