Closed Bug 1217001 Opened 7 years ago Closed 7 years ago

Refactor BytecodeEmitter::emitVariables

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(14 files)

4.14 KB, patch
shu
: review+
Details | Diff | Splinter Review
9.25 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.46 KB, patch
Details | Diff | Splinter Review
2.08 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.31 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.07 KB, patch
Details | Diff | Splinter Review
4.11 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.79 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.88 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.41 KB, patch
shu
: review+
Details | Diff | Splinter Review
11.20 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.35 KB, patch
shu
: review+
Details | Diff | Splinter Review
9.68 KB, patch
Details | Diff | Splinter Review
5.02 KB, patch
shu
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
In particular, before this patch, emitNormalFor expected `emitTree(|var x = 3;|)` to leave the value `3` on the operand stack. This is totally weird and must die.
Attachment #8676946 - Flags: review?(shu)
Attachment #8676989 - Flags: review?(shu)
Blocks: 1217099
Blocks: 1216623
Comment on attachment 8676877 [details] [diff] [review]
Refactor BytecodeEmitter::variables. Part 1: preliminaries

Review of attachment 8676877 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4258,5 @@
>                          return false;
>                      if (!emitInitializeDestructuringDecls(pn->getOp(), pn2))
>                          return false;
>                  }
> +                continue;

Note to self: this change is okay because in this case, we know pn->pn_count == 1 and so pn2->pn_next == nullptr.
Attachment #8676877 - Flags: review?(shu) → review+
Comment on attachment 8676885 [details] [diff] [review]
Part 2: Rename two local variables and improve some old comments

Review of attachment 8676885 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4231,5 @@
> +        if (!binding->isKind(PNK_NAME)) {
> +            if (binding->isKind(PNK_ARRAY) || binding->isKind(PNK_OBJECT)) {
> +                // Destructuring BindingPattern in a `for` loop head:
> +                //     for (let [x, y] of pts) ...;
> +                //     a = [x*y for (let [x, y] of pts)];

Add a note saying comprehension syntax is legacy.
Attachment #8676885 - Flags: review?(shu) → review+
Attachment #8676888 - Flags: review?(shu) → review+
Attachment #8676893 - Flags: review?(shu) → review+
Attachment #8676897 - Flags: review?(shu) → review+
Attachment #8676898 - Flags: review?(shu) → review+
Comment on attachment 8676946 [details] [diff] [review]
Part 7: Change BytecodeEmitter::emitNormalFor() to decouple it from weird expectations about BytecodeEmitter::emitVariables()

Review of attachment 8676946 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5609,5 @@
>       */
>      unsigned noteIndex;
>      if (!newSrcNote(SRC_FOR, &noteIndex))
>          return false;
> +    if (!emit1(JSOP_NOP))

Why do we emit this NOP?

@@ +5702,5 @@
>      if (!setSrcNoteOffset(noteIndex, 2, offset() - tmp))
>          return false;
>  
>      /* If no loop condition, just emit a loop-closing jump. */
> +    JSOp op = forHead->pn_kid2 ? JSOP_IFNE : JSOP_GOTO;

Is the emitJump below the only use of this? Maybe just inline the expression?
Attachment #8676946 - Flags: review?(shu) → review+
Comment on attachment 8676980 [details] [diff] [review]
Part 8: Eliminate all uses of PNX_POPVAR

Review of attachment 8676980 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice.
Attachment #8676980 - Flags: review?(shu) → review+
Comment on attachment 8676989 [details] [diff] [review]
Part 9: Remove PNX_POPVAR

Review of attachment 8676989 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ -5015,5 @@
>          if (!kid)
>              return null();
> -        kid->pn_xflags = PNX_POPVAR;
> -
> -        kid = MatchOrInsertSemicolonAfterExpression(tokenStream) ? kid : nullptr;

lol is this for real
Attachment #8676989 - Flags: review?(shu) → review+
Attachment #8676990 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #16)
> Add a note saying comprehension syntax is legacy.

Done.

> Why do we emit this NOP?

Good question. I'll revisit it; this patch was just taking the shortest path.
Attachment #8676999 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #17)
> ::: js/src/frontend/BytecodeEmitter.cpp
> >      unsigned noteIndex;
> >      if (!newSrcNote(SRC_FOR, &noteIndex))
> >          return false;
> > +    if (!emit1(JSOP_NOP))
> 
> Why do we emit this NOP?

OK. It's to hang the SRC_FOR source note on. Ion requires this source note. I think we can eliminate the NOP, but the code here and in IonBuilder::forLoop is slightly simplified by the JSOP_NOP instruction, because the size of the instruction is always 1.

If you like I can file a follow-up bug and do this. Otherwise I'm going to leave it.

> > +    JSOp op = forHead->pn_kid2 ? JSOP_IFNE : JSOP_GOTO;
> 
> Is the emitJump below the only use of this? Maybe just inline the expression?

Yep. Done.
(In reply to Jason Orendorff [:jorendorff] from comment #21)
> (In reply to Shu-yu Guo [:shu] from comment #17)
> > ::: js/src/frontend/BytecodeEmitter.cpp
> > >      unsigned noteIndex;
> > >      if (!newSrcNote(SRC_FOR, &noteIndex))
> > >          return false;
> > > +    if (!emit1(JSOP_NOP))
> > 
> > Why do we emit this NOP?
> 
> OK. It's to hang the SRC_FOR source note on. Ion requires this source note.
> I think we can eliminate the NOP, but the code here and in
> IonBuilder::forLoop is slightly simplified by the JSOP_NOP instruction,
> because the size of the instruction is always 1.
> 
> If you like I can file a follow-up bug and do this. Otherwise I'm going to
> leave it.
> 

Ah okay, no need to file a bug. I'm fine with leaving this in.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ff10c36ebe8aa2b7d8ddbea1943dffa8ec4798
Bug 1217001 - Refactor BytecodeEmitter::variables. Part 1: preliminaries. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a07f584d3bccc2a774fd28d9c2789ba20fcf4cc
Bug 1217001 - Part 2: Rename two local variables and improve some old comments. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d67895e08fb92e9057532220021aeee1b9516fd
Bug 1217001 - Part 3: Remove one goto statement. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/310949c85ae5c26261769444557189190063d8cb
Bug 1217001 - Part 4: Improve the comments on VarEmitOption. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/264a443f5b066271214455b212d1327ab8770eec
Bug 1217001 - Part 5: Further revise control structure in BytecodeEmitter::emitVariables(). r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9964e08cec60cc94fdfc5f8fb625e6fb7a791c
Bug 1217001 - Part 6: Eliminate some `continue` statements. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e96d501788e41f3d7d305dd3cff384e8dd3ae7
Bug 1217001 - Part 7: Change BytecodeEmitter::emitNormalFor() to decouple it from weird expectations about BytecodeEmitter::emitVariables(). r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0dcdea04ffe410ee31a565b80de1e1ab4c9093
Bug 1217001 - Part 8: Eliminate all uses of PNX_POPVAR. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d59de65bcd8300135a7abbd1aef8fb4a526592
Bug 1217001 - Part 9: Remove PNX_POPVAR. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb80a132a0c33b0abb63b9026888e615ae712ae
Bug 1217001 - Part 10: Delete redundant boolean argument. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/119bb65c0ae504029651c622134906a787cbdc8b
Bug 1217001 - Part 11: Get rid of the last goto in BytecodeEmitter::emitVariables(). r=shu.
You need to log in before you can comment on or make changes to this bug.