1217001 - Refactor BytecodeEmitter::emitVariables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Refactor BytecodeEmitter::variables. Part 1: preliminaries

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Rename two local variables and improve some old comments

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Remove one goto statement

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Remove one goto statement (with diff -b)

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 4: Improve the comments on VarEmitOption

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Further revise control structure in BytecodeEmitter::emitVariables()

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Further revise control structure in BytecodeEmitter::emitVariables() (with diff -b)

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 6: Eliminate some `continue` statements

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Part 7: Change BytecodeEmitter::emitNormalFor() to decouple it from weird expectations about BytecodeEmitter::emitVariables()

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.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 8: Eliminate all uses of PNX_POPVAR

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 9: Remove PNX_POPVAR

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 10: Delete redundant boolean argument

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Part 11: Get rid of the last goto in BytecodeEmitter::emitVariables()

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Part 11: Get rid of the last goto in BytecodeEmitter::emitVariables(). (with diff -b)

Comment 15

[Shu-yu Guo [:shu]]

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.

Comment 16

[Shu-yu Guo [:shu]]

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.

Comment 17

[Shu-yu Guo [:shu]]

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?

Comment 18

[Shu-yu Guo [:shu]]

Comment on attachment 8676980 [details] [diff] [review]
Part 8: Eliminate all uses of PNX_POPVAR

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

Very nice.

Comment 19

[Shu-yu Guo [:shu]]

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

Comment 20

[Jason Orendorff [:jorendorff]]

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

Comment 21

[Jason Orendorff [:jorendorff]]

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

Comment 22

[Shu-yu Guo [:shu]]

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

Comment 23

[Jason Orendorff [:jorendorff]]

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.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4ff10c36ebe
https://hg.mozilla.org/mozilla-central/rev/6a07f584d3bc
https://hg.mozilla.org/mozilla-central/rev/0d67895e08fb
https://hg.mozilla.org/mozilla-central/rev/310949c85ae5
https://hg.mozilla.org/mozilla-central/rev/264a443f5b06
https://hg.mozilla.org/mozilla-central/rev/6b9964e08cec
https://hg.mozilla.org/mozilla-central/rev/b5e96d501788
https://hg.mozilla.org/mozilla-central/rev/1e0dcdea04ff
https://hg.mozilla.org/mozilla-central/rev/f7d59de65bcd
https://hg.mozilla.org/mozilla-central/rev/8bb80a132a0c
https://hg.mozilla.org/mozilla-central/rev/119bb65c0ae5