648175 - Get rid of JSOP_FOR* opcodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Currently we compile this

    for (TARGET in OBJ)
       STMT

to something like:

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

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);
        STMT
    }

Jaeger already treats the FOR* opcodes this way, and it'll be simpler to analyze.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1 - not working

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.

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2 - decompiler not working

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.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: WIP 3 - not quite working in decompiler

This fixes several runtime bugs in WIP 2 and gets some decompiler basics working. I'm still seeing some assertions under DecompileValueGenerator.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: WIP 4 - failing 6 tests

Now with enhanced not-crashing.

The patch is around -250 lines net.

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v5

actually attach the patch

Comment 8

[Brendan Eich [: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.

/be

Comment 9

[Jason Orendorff [:jorendorff]]

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

Try server hates this. Withdrawing review while I debug.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v6

(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.)

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 526049 [details] [diff] [review]
v6

I don't think this really needs brendan's time. Reassigning to dvander, but please feel free to bounce it to someone else...

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 526049 [details] [diff] [review]
v6

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();
>            END_CASE(JSOP_ARGUMENTS)
>  
> +          BEGIN_CASE(JSOP_ITERNEXT)
> +            iterNext(PC[1]);

Could this use GET_INT8(PC)?

Comment 13

[Jason Orendorff [:jorendorff]]

Yay! Thanks for the review. I'll rebase when I get back from vacation (14 June).

Comment 14

[Jason Orendorff [:jorendorff]]

Did I say 14 June? Off-by-one error.

Trying it out now: http://tbpl.mozilla.org/?tree=Try&rev=fc1965c1c770

Comment 15

[Jeff Walden [:Waldo]]

http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b2d134ef9d34

turned B machines a bit orange, alas, so I had to back it out.  :-(

Comment 16

[Joe Drew (not getting mail)]

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.

Comment 17

[Jason Orendorff [:jorendorff]]

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.

Comment 18

[Jason Orendorff [:jorendorff]]

Correction, it wasn't backed out a third time! A backout was merged across it, but it was spared.

http://hg.mozilla.org/mozilla-central/rev/938c1a177114

Phew.