Remove the bytecode main/prologue split

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: shu, Assigned: jandem)

Tracking

({triage-deferred})

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

After the rewrite, the prologue can be removed. Things will be emitted in the right order up front, no need to switch to prologue due to function hoisting.
Depends on: 1263355
One problem is that the Debugger considers ops in the prologue to be unobservable, which affects the ability to set a breakpoint on the op. If we were to get rid of the prologue, we need a more robust notion of which ops can be breakpoint targets.
(In reply to Shu-yu Guo [:shu] from comment #1)
> One problem is that the Debugger considers ops in the prologue to be
> unobservable, which affects the ability to set a breakpoint on the op. If we
> were to get rid of the prologue, we need a more robust notion of which ops
> can be breakpoint targets.

Maybe a JSOP_START or JSOP_BEGIN op, where we currently switch to main?
I thought we only allowed breakpoints on bytecode which have new-line information?  Does the prologue have any line notes?
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I thought we only allowed breakpoints on bytecode which have new-line
> information?  Does the prologue have any line notes?

The FlowGraphSummary used by Debugger explicitly uses offsets relative to script->main(), not script->code() right now.
Keywords: triage-deferred
Priority: -- → P3

Hah, I was just filing a bug for this. I prototyped some patches for this that are green so far, the main issue is DEFFUN ops in global/eval scripts need to be emitted upfront. My patches store these functions in a Vector in {Global,Eval}SharedContext.

https://hg.mozilla.org/try/rev/5f5e70e89ebcec97b77c7055bc20659321d3cd4e
https://hg.mozilla.org/try/rev/b35443b316ecee7535b9cec7e3ccc462df16a23b

Emitting everything linearly will simplify some IC work I want to do later.

arai, what do you think?

Flags: needinfo?(arai.unmht)

Looks great :)

I don't see any issue so far,
but I want to make sure how it affects source notes for prologue bytecodes, will look into the details shortly.
(it's related to bug 1519005 patch)

See Also: → 1519005

so, in the patch, hoisted/top-level functions' bytecode are emitted in the different place than where the source notes are emitted (emitHoistedFunctionsInList)
now I think we really don't need the source notes for them.

also, hoisted/top-level function's JSFunction/FunctionBox are still modified inside BytecodeEmitter::emitFunction.

I wonder if we can factor out that part into the different function and use it both from both BytecodeEmitter::{emitFunction,defineTopLevelFunctions}, and completely skip hoisted/top-level functions in emitFunction.
but maybe it's for other bug (part of bug 1473796 maybe?).

anyway, the patch looks good to me :D

Blocks: 1519005, 1473796
Flags: needinfo?(arai.unmht)
See Also: 1519005

Thanks arai.

I didn't see emitHoistedFunctionsInList yesterday, I think we can use that for this too. I'll try this today.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d260750a87e
part 1 - Emit hoisted top-level functions directly in the prologue, remove switchToPrologue. r=arai
https://hg.mozilla.org/integration/autoland/rev/83d6478d2611
part 2 - Fold EmitSection into BytecodeEmitter, remove prologue/main split. r=arai
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.