Closed Bug 905091 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
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.
Attachment #790125 - Flags: review?(bhackett1024)
Summary: IonMonkey: LSRA: Don't insert movegroups between an instruction and it's OsiPoint → IonMonkey: LSRA: Don't insert movegroups between an instruction and its OsiPoint
Blocks: 905148
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.
Attachment #790125 - Flags: review?(bhackett1024) → review+
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.
Whiteboard: [leave open]
Attached patch Part 2Splinter Review
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.
Attachment #790787 - Flags: review?(bhackett1024)
Attachment #790787 - Flags: review?(bhackett1024) → review+
Duplicate of this bug: 903967
Blocks: 905723
https://hg.mozilla.org/mozilla-central/rev/a655e2613667
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/1b16c2c729b9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.