Last Comment Bug 648175 - Get rid of JSOP_FOR* opcodes
: Get rid of JSOP_FOR* opcodes
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
-- normal (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
: Jason Orendorff [:jorendorff]
Depends on: 672854 672892 696492
Blocks: 647620
  Show dependency treegraph
Reported: 2011-04-06 17:32 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-10-21 14:32 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP 1 - not working (104.63 KB, patch)
2011-04-06 17:41 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 2 - decompiler not working (69.60 KB, patch)
2011-04-09 18:44 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 3 - not quite working in decompiler (81.49 KB, patch)
2011-04-10 09:50 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 4 - failing 6 tests (92.05 KB, patch)
2011-04-10 15:57 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v5 (100.75 KB, patch)
2011-04-12 17:05 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v6 (100.75 KB, patch)
2011-04-14 10:45 PDT, Jason Orendorff [:jorendorff]
dvander: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2011-04-06 17:32:47 PDT
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.
Comment 1 User image Jason Orendorff [:jorendorff] 2011-04-06 17:41:43 PDT
Created attachment 524321 [details] [diff] [review]
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 User image Jason Orendorff [:jorendorff] 2011-04-09 07:17:37 PDT
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 User image Jason Orendorff [:jorendorff] 2011-04-09 18:44:44 PDT
Created attachment 524903 [details] [diff] [review]
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 User image Jason Orendorff [:jorendorff] 2011-04-10 09:50:43 PDT
Created attachment 524962 [details] [diff] [review]
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 User image Jason Orendorff [:jorendorff] 2011-04-10 15:57:27 PDT
Created attachment 524999 [details] [diff] [review]
WIP 4 - failing 6 tests

Now with enhanced not-crashing.

The patch is around -250 lines net.
Comment 6 User image Jason Orendorff [:jorendorff] 2011-04-12 17:04:14 PDT
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 User image Jason Orendorff [:jorendorff] 2011-04-12 17:05:18 PDT
Created attachment 525560 [details] [diff] [review]

actually attach the patch
Comment 8 User image Brendan Eich [:brendan] 2011-04-12 18:25:24 PDT
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 9 User image Jason Orendorff [:jorendorff] 2011-04-13 23:24:44 PDT
Comment on attachment 525560 [details] [diff] [review]

Try server hates this. Withdrawing review while I debug.
Comment 10 User image Jason Orendorff [:jorendorff] 2011-04-14 10:45:01 PDT
Created attachment 526049 [details] [diff] [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.)
Comment 11 User image Jason Orendorff [:jorendorff] 2011-05-09 09:22:41 PDT
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...
Comment 12 User image David Anderson [:dvander] 2011-06-02 11:41:16 PDT
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)?
Comment 13 User image Jason Orendorff [:jorendorff] 2011-06-02 14:26:09 PDT
Yay! Thanks for the review. I'll rebase when I get back from vacation (14 June).
Comment 14 User image Jason Orendorff [:jorendorff] 2011-07-14 09:48:45 PDT
Did I say 14 June? Off-by-one error.

Trying it out now:
Comment 15 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-15 22:57:41 PDT

turned B machines a bit orange, alas, so I had to back it out.  :-(
Comment 16 User image Joe Drew (not getting mail) 2011-07-16 18:46:44 PDT
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 User image Jason Orendorff [:jorendorff] 2011-08-01 07:53:26 PDT
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 User image Jason Orendorff [:jorendorff] 2011-08-01 09:59:31 PDT
Correction, it wasn't backed out a third time! A backout was merged across it, but it was spared.


Note You need to log in before you can comment on or make changes to this bug.