Remove SKIP_POP_AFTER_SET

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
This is an interpreter optimization that allows it to skip POP opcodes instead of pushing a value and then immediately popping it.  This optimization really isn't necessary in the presence of JITs, and seems to make trace recording and other interpreter instrumentation more complicated, judging from the (scary) code snippet below from LeaveTree.

            /*
             * JSOP_SETELEM can be coalesced with a JSOP_POP in the interpeter.
             * Since this doesn't re-enter the recorder, the post-state snapshot
             * is invalid. Fix it up here.
             */
            if (op == JSOP_SETELEM && JSOp(regs->pc[JSOP_SETELEM_LENGTH]) == JSOP_POP) {
                regs->sp -= js_CodeSpec[JSOP_SETELEM].nuses;
                regs->sp += js_CodeSpec[JSOP_SETELEM].ndefs;
                regs->pc += JSOP_SETELEM_LENGTH;
                op = JSOP_POP;
            }

Comment 1

6 years ago
Yeah, opcode fusing has been no end of troubles in the tracer.
(Assignee)

Updated

6 years ago
Assignee: general → evilpies
(Assignee)

Comment 2

6 years ago
Created attachment 570344 [details] [diff] [review]
remove the marcos

I also included the changes to jsop_toid, hope this is okay.
Attachment #570344 - Flags: review?(bhackett1024)
(Reporter)

Comment 3

6 years ago
Comment on attachment 570344 [details] [diff] [review]
remove the marcos

Can you remove the SKIP_POP_AFTER_SET tracer special casting in comment 0?  It might be better to wait on this bug until the tracer is completely removed, I don't know if there are other ways SKIP_POP_AFTER_SET can subtly break tracing/recording behavior.
Attachment #570344 - Flags: review?(bhackett1024)
(Assignee)

Updated

6 years ago
Depends on: 698201
(Assignee)

Comment 4

6 years ago
Comment on attachment 570344 [details] [diff] [review]
remove the marcos

So let's try again
Attachment #570344 - Flags: review?(bhackett1024)
(Reporter)

Updated

6 years ago
Attachment #570344 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bc7ff681a4ae
https://hg.mozilla.org/mozilla-central/rev/bc7ff681a4ae
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.