1766730 - Remove JSOp::Retsub

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Iain Ireland [:iain]]

In bug 885514, we rewrote bytecode emission for try-finally to enable simple instances to be compiled in Warp. The main obstacle to supporting more cases is JSOp::Retsub, which pops a resume index from the stack and jumps to the corresponding target. This is hard to support in Warp because:

1. Figuring out which blocks a retsub can reach needs dataflow analysis that doesn't play nicely with WarpBuilder.
2. In non-local exit cases (return, break, continue) where we have to do some more work after the finally, the retsub goes backwards to earlier bytecode, which may create a spaghetti CFG and confuse eg dominator analysis. (The "more work" may include closing iterators, exiting scopes, and so on.)

After failing with a variety of incremental approaches, I have concluded that the best approach is to take the nuclear solution and remove JSOp::Retsub entirely, replacing it with a desugared tableswitch. Using a backwards JSOp::Goto as the body of the switch case would break the disassembler and bytecode analysis (which expect only loop edges to go backwards), but we can solve this problem by moving the "more work" that needs to be done after the finally into the switch case itself. The eventual catch, break, or return is guaranteed not to need a backward jump.

Once this is done, try-finally will Just Work in Warp with no additional effort.

Comment 1

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Remove NonLocalExitControl::Throw r=jandem

This has been dead code since bug 1228841 landed.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Refactor emitReturn r=jandem

This is in preparation for the next patch.

The current code optimistically emits JSOp::Return and then changes it to JSOp::SetRval if necessary. Given that this code is being split up, it seemed better to conservatively emit JSOp::SetRval and treat the conversion to JSOp::Return as an optimization.

Depends on D145028

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Emit final jump/return inside NonLocalExitControl r=jandem

This makes it easier for subsequent patches to generate the final jump/return inside the finally block.

Depends on D145029

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Move NonLocalExitControl into BytecodeControlStructures.cpp r=jandem

In a subsequent patch, TryFinallyControl will need to be able to see NonLocalExitControl.

Depends on D145030

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Add InternalSwitchEmitter r=jandem

This approach is copied from InternalIfEmitter.

Depends on D145031

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Finish non-local jumps in finally block r=jandem

This is the patch that everything has been building towards.

Instead of pushing a resume index and using retsub, we push a continuation index, which is used as the discriminant in a tableswitch. Index 0 is reserved for the fallthrough case. Other continuations store a kind (break, continue, or return) and a target control (the continued / broken loop; nullptr for returns). Because of nesting, the target is guaranteed to outlive the finally control.

We have to be a little careful in emitNonLocalJump to skip past the switch and finally that we're currently emitting.

Depends on D145032

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Deduplicate continuations r=jandem

This is technically quadratic in the number of continuations, but the worst-case number of continuations is O(loop nesting depth), and doesn't seem to exceed 2 in real-world code.

Depends on D145033

Comment 8

[Iain Ireland [:iain]]

Attachment: Bug 1766730: Remove JSOp::Retsub and JSOp::ResumeIndex r=jandem

Depends on D145034

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fed7a291a827
Remove NonLocalExitControl::Throw r=jandem
https://hg.mozilla.org/integration/autoland/rev/6e40e83390a8
Refactor emitReturn r=jandem
https://hg.mozilla.org/integration/autoland/rev/0f7ffba82f4d
Emit final jump/return inside NonLocalExitControl r=jandem
https://hg.mozilla.org/integration/autoland/rev/45228ffd33c7
Move NonLocalExitControl into BytecodeControlStructures.cpp r=jandem
https://hg.mozilla.org/integration/autoland/rev/8aec28151390
Add InternalSwitchEmitter r=jandem
https://hg.mozilla.org/integration/autoland/rev/0db6f17a0d73
Finish non-local jumps in finally block r=jandem
https://hg.mozilla.org/integration/autoland/rev/3be7d500db82
Deduplicate continuations r=jandem
https://hg.mozilla.org/integration/autoland/rev/51ef6e4c6939
Remove JSOp::Retsub and JSOp::ResumeIndex r=jandem

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/fed7a291a827
https://hg.mozilla.org/mozilla-central/rev/6e40e83390a8
https://hg.mozilla.org/mozilla-central/rev/0f7ffba82f4d
https://hg.mozilla.org/mozilla-central/rev/45228ffd33c7
https://hg.mozilla.org/mozilla-central/rev/8aec28151390
https://hg.mozilla.org/mozilla-central/rev/0db6f17a0d73
https://hg.mozilla.org/mozilla-central/rev/3be7d500db82
https://hg.mozilla.org/mozilla-central/rev/51ef6e4c6939