905091 - IonMonkey: LSRA: Don't insert movegroups between an instruction and its OsiPoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

For try-catch (and later on the debugger, bug 716647) we want to use a combination of a call's safepoint + the snapshot of the OsiPoint that follows it to reconstruct the stack.

For this to work, we need an invariant that nothing between the call and its OsiPoint modifies any registers that are stored in the safepoint. I have a patch to add runtime checks for this in debug builds, and it found one issue: LSRA can insert a movegroup between an instruction and its OsiPoint. I don't think it's a problem for try-catch atm, but it may avoid problems in other cases.

The backtracking allocator already has code for this, so I moved it to the base class and reused it. As the comment says, it's a bit disgusting, but it's pretty easy to implement in the allocators and having this invariant makes implementing things like try-catch a lot nicer/simpler.

This patch fixes all jit-test failures I got with the patch to add runtime checks.

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 790125 [details] [diff] [review]
Patch

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

::: js/src/jit/RegisterAllocator.h
@@ +357,5 @@
> +        // Compute the shortest interval that captures vregs defined by ins.
> +        // Watch for instructions that are followed by an OSI point and/or Nop.
> +        // If moves are introduced between the instruction and the OSI point then
> +        // safepoint information for the instruction may be incorrect. This is
> +        // pretty disgusting and should be fixed somewhere else in the compiler.

Now that this is ensconced in RegisterAllocator.h and not a backtracking allocator special case, maybe the last sentence in this comment can be removed.

Comment 2

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a655e2613667

Try: https://tbpl.mozilla.org/?tree=Try&rev=81f685ee31cc

Will remove that sentence as part of the second part.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2

This patch adds an assert to CodeGenerator::visitOsiPoint to ensure there are no instructions between an instruction and its OsiPoint.

It also inserts spill-at-definition moves after the OsiPoint. This should fix bug 903967 and maybe similar crashes.

Passes jit-tests --ion on x86/x64.

Comment 5

[Jan de Mooij [:jandem]]

Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b16c2c729b9

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a655e2613667

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b16c2c729b9