Closed Bug 648175 Opened 13 years ago Closed 13 years ago

Get rid of JSOP_FOR* opcodes


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: jorendorff, Assigned: jorendorff)


(Blocks 1 open bug)



(1 file, 5 obsolete files)

Currently we compile this

    for (TARGET in OBJ)

to something like:

    for (let __iter = JSOP_ITER(OBJ); JSOP_MOREITER(__iter);) {
        JSOP_FORNAME(&TARGET, __iter);

where JSOP_FORNAME could end up being optimized to some other opcode, or deoptimized to JSOP_NEXTELEM in certain cases (such as a destructuring TARGET).

We should just always use JSOP_NEXTELEM, and rename it to JSOP_NEXT:

    for (let __iter = JSOP_ITER(OBJ); JSOP_MOREITER(__iter);) {
        TARGET = JSOP_NEXT(__iter);

Jaeger already treats the FOR* opcodes this way, and it'll be simpler to analyze.
Attached patch WIP 1 - not working (obsolete) — Splinter Review
Remaining to be done:

  - I need to flip the TARGET opcodes from GET to SET on some parse nodes
    before emitting the assignment. (Gross, but it would work, I think.)

  - Decompiler.

This goes on top of bug 646807 and bug 646820 ...which I really should land sometime.

Sorry if this patch is disgusting; I know a bit more about the emitter now than I did when I started on this.
Taking. That patch has too much flailing around in it. There's no way that was going to succeed all the way through to the decompiler. Writing a new one.
Assignee: general → jorendorff
Attached patch WIP 2 - decompiler not working (obsolete) — Splinter Review
This is much better. I have not touched the decompiler, so that will be super crashy -- but a wide variety of for-in loops at least get through the parser and interpreter.
Attachment #524321 - Attachment is obsolete: true
This fixes several runtime bugs in WIP 2 and gets some decompiler basics working. I'm still seeing some assertions under DecompileValueGenerator.
Attachment #524903 - Attachment is obsolete: true
Attached patch WIP 4 - failing 6 tests (obsolete) — Splinter Review
Now with enhanced not-crashing.

The patch is around -250 lines net.
Attachment #524962 - Attachment is obsolete: true
Comment on attachment 524999 [details] [diff] [review]
WIP 4 - failing 6 tests

Whew. This passes tests. -222 lines net.

Executive summary: for-in loop heads switch from binary to ternary nodes. kid1 is the decl, if any, else NULL. kid2 is an lhs that can be passed to the emitter's assignment code (any names in it can be passed to BindNameForSlot). kid3 is the rhs.

If you prefer I could put the complexity in the emitter--but it would be pretty much a total rewrite of the patch. I chose copying nodes because it's trivial to copy nodes.

A style question. Current parser style is like this (pseudocode):
    pn = BinaryNode::create(JSOP_FOO, tc);
    pn->pn_left = parseLeft();
    pn->pn_right = parseRight();
    return pn;

This patch contains some code like this:
    lhs = parseLeft();
    rhs = parseRight();
    return BinaryNode::create(JSOP_FOO, lhs, rhs, tc);

I much prefer that style, but it's subjective. Maybe it's just me? Or maybe there's some GC hazard I'm not seeing? Either way it would be no trouble to change it back.

cdleary or dvander, feel free to steal review.
Attachment #524999 - Flags: review?(brendan)
Attached patch v5 (obsolete) — Splinter Review
actually attach the patch
Attachment #524999 - Attachment is obsolete: true
Attachment #524999 - Flags: review?(brendan)
Attachment #525560 - Flags: review?(brendan)
Cloning nodes to make ASTs that are easier to process in the emitter is great, that's why I added CloneParseTree.

I think the motivation for calling BinaryNode::create first and then parsing left and right was to avoid the lhs and rhs temps. But your preferred style is nicer given those temps. No problem with GC afaict.

Will review when I can, some time this week.

Comment on attachment 525560 [details] [diff] [review]

Try server hates this. Withdrawing review while I debug.
Attachment #525560 - Flags: review?(brendan)
Attached patch v6Splinter Review
(Easy two-byte fix for boneheaded bug: the parameter 'uint32 offset' needs to be int32 if you plan to do sp[-offset] on 64-bit platforms.)
Attachment #525560 - Attachment is obsolete: true
Attachment #526049 - Flags: review?(brendan)
Comment on attachment 526049 [details] [diff] [review]

I don't think this really needs brendan's time. Reassigning to dvander, but please feel free to bounce it to someone else...
Attachment #526049 - Flags: review?(brendan) → review?(dvander)
Comment on attachment 526049 [details] [diff] [review]

Review of attachment 526049 [details] [diff] [review]:

Sorry for the really late review. This patch is awesome.

::: js/src/jsparse.cpp
@@ +5080,5 @@
> +                JSParseNode *tag = CloneParseTree(opn2->pn_left, tc);
> +                if (!tag)
> +                    return NULL;
> +                JSParseNode *target = CloneLeftHandSide(opn2->pn_right, tc);
> +                if (!target)

Can this recursion go deep enough to warrant JS_CHECK_RECURSION?

::: js/src/methodjit/Compiler.cpp
@@ +1014,5 @@
>              frame.pushSynced();
> +            iterNext(PC[1]);

Could this use GET_INT8(PC)?
Attachment #526049 - Flags: review?(dvander) → review+
Yay! Thanks for the review. I'll rebase when I get back from vacation (14 June).
Did I say 14 June? Off-by-one error.

Trying it out now:

turned B machines a bit orange, alas, so I had to back it out.  :-(
This bug's second landing (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
I landed it again on 19 July, and it got backed out again, only this time nobody did anything at all to notify anyone.

This patch contains a lot of hard work, and it is correct. It has been backed out three times now for peripheral issues. It seems to be cursed. Unbitrotting again.
Correction, it wasn't backed out a third time! A backout was merged across it, but it was spared.

Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.