1456404 - Add helper classes to emit bytecode for loops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Tooru Fujisawa [:arai]]

Just like IfThenElseEmitter (bug 1147371) and TryEmitter (bug 1322069) and SwitchEmitter (bug 1456006)
having classes for for loops, while loops, etc, should help bug 1455548.

currently a class for c-style loop is almost ready,
but I think it's better preparing classes for all loops and factor out common code,
and land them at once.

Comment 1

[Tooru Fujisawa [:arai]]

fwiw, here's CStyleForEmitter
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73df751473b4d4bfb13bb4b6cf5274c6a3bd068

Comment 2

[Tooru Fujisawa [:arai]]

similar problem to bug 1456006 comment #1.

Currently we're emitting bytcode in the different order than the source code's order in some case.

for example, c-style for's byetcode order is:
  1. init
  2. body
  3. update
  4. cond

while the source code's order and BinAST multipart file's current order are:
  1. init
  2. cond
  3. update
  4. body

Comment 3

[Tooru Fujisawa [:arai]]

filed https://github.com/binast/ecmascript-binary-ast/issues/35

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move loop related bytecode/offset handling to LoopControl.

Given that there are several code shared between helper classes for loops (for/while), moved those methods and related fields to LoopControl.
Also, added some accessors for offsets, which is used by subsequent patches.

on BytecodeEmitter's side, it's mostly simple replacement to just call LoopControl's methods,
except BytecodeEmitter::getOffsetForLoop which was part of BytecodeEmitter::{emitLoopHead,emitLoopEntry} and moved out of them.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add CForEmitter.

Added CForEmitter for `for(;;) {}` loop.
The usage is in the comment above the class declaration.

In addition to adding helper class like SwitchEmitter,
I added SrcNote::For which holds field indices for SRC_FOR, which corresponds to things added in bug 1477621.

there are some unused code/comment about SRC_FOR, which will be removed in Part 7, after surrounding code got cleaned up.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add ForInEmitter.

Added ForInEmitter for `for (... in ...) {}` loop.
Mostly simple code move.

I've added some comment about the TDZCheckCache for iterated value (tdzCacheForIteratedValue_),
based on bug 1368360 and https://mozilla.logbot.info/jsapi/20170614#c848252
tcampbell, can you check if the comment in the patch is correct?

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add ForOfEmitter.

Added ForOfEmitter for `for (... of ...) {}` loop.

Also added BytecodeEmitter::emitCall variants which takes const Maybe<uint32_t>& instead of ParseNode*, and also changed BytecodeEmitter::emitIteratorNext to take const Maybe<uint32_t>& instead of ParseNode*, to use it from helper class which doesn't use ParseNode.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Add WhileEmitter.

Added WhileEmitter for `while (...) {}` loop.

SRC_WHILE is used both from while and do-while, and do-while uses it in the different way.
I'll add another SrcNote class for it, and then cleanup it in bug 1477896, by adding SRC_DO_WHILE.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Add DoWhileEmitter.

Added DoWhileEmitter for `do {} while (...)` loop.

As mentioned above, currently SRC_WHILE is shared between while, and do-while uses 2 SRC_WHILE notes in order to add 2 offsets, I've added SrcNote::{DoWhile1,DoWhile2} for them, which will shortly be merged in bug 1477896.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Remove unused code/comment for SRC_FOR.

Removed comments about offsetBias, and the JSOP_POP case for SRC_FOR,
and also removed ControlFlowGenerator::maybeLoop and embedded the code into ControlFlowGenerator::snoopControlFlow given that it's already small, and SRC_WHILE case there (which is do-while) will be moved outside of it in bug 1477896.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8994404 [details] [diff] [review]
Part 3: Add ForInEmitter.

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

::: js/src/frontend/ForInEmitter.h
@@ +63,5 @@
> +    // in Ion work properly.
> +    // In term of the execution order, the TDZCheckCache for the iterated value
> +    // dominates the one for the iteration body, that means the checks in the
> +    // iteration body is dead, and we can optimize them away.  But the sanity
> +    // check in Ion doesn't know it's dead.

The original issue was super subtle and still confuses me. Looking at the comment in front of TDZCheckCache, it is pretty clear that a new cache should be used when entering a lexical (which this case is). Here is an alternate comment block for you to consider.

// Cache for the iterated value, which is separated from both enclosing block
// and the iteration body. The cache for the iteration body is inside
// `loopInfo_`.
//
// The binding for the iterated value is recomputed each loop in a new lexical
// environment, but with the same name. We might be tempted to avoid this avoid
// this extra TDZCheckCache scope since a TDZ violation here will prevent the
// body code from running. The current TDZCheckCache is defined in terms of
// lexical bindings (vs just names) and in practice this trips up IonMonkey
// sanity checks. See Bug 1368360 for more context.

(Clearing feedback, but feel free to land with your original comment if that is clearer than mine)

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8994402 [details] [diff] [review]
Part 1: Move loop related bytecode/offset handling to LoopControl.

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

::: js/src/frontend/BytecodeControlStructures.h
@@ +123,5 @@
> +    //
> +    //   breakTarget:
> +    //
> +    // `continueTarget` can be placed in arbitrary place by calling
> +    // `setContinueTarget` or `emitContinueTarget` (see commentd above them for

comment

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5129,5 @@
>          return false;
>      if (!emit1(JSOP_ISNOITER))                            // ITER NEXTITERVAL? ISNOITER
>          return false;
>  
> +    if (!loopInfo.emitBackwardJump(this, JSOP_IFEQ))      // ITER NEXTITERVAL

emitBackwardJump is a bit too general of a name in light of the loop-centric things now being stored, IMO.  What about emitRestartLoop?  emitContinueLoop?  emitLoopAgain?  Something along any of these lines, or something.

@@ +5292,5 @@
>              current->lastColumn = 0;
>          }
>      }
>  
> +    ptrdiff_t condOffset = offset();

But but but tmp3 was clearly a better name!

@@ +5684,5 @@
>       *
>       * Be careful: We must set noteIndex2 before noteIndex in case the noteIndex
>       * note gets bigger.
>       */
> +    if (!setSrcNoteOffset(noteIndex2, 0, loopInfo.backwardJumpOffsetFromLoopHead()))

Similar to the backward-jump renaming idea, this function should be loopEndOffsetFromLoopHead or something like that, seems like.

@@ +5686,5 @@
>       * note gets bigger.
>       */
> +    if (!setSrcNoteOffset(noteIndex2, 0, loopInfo.backwardJumpOffsetFromLoopHead()))
> +        return false;
> +    // +1 for NOP above.

You mean JSOP_JUMPTARGET emitted by emitBackwardJump?  If so, and I think you do, then say that, not "NOP".

Comment 13

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Jeff Walden [:Waldo] from comment #12)
> @@ +5686,5 @@
> >       * note gets bigger.
> >       */
> > +    if (!setSrcNoteOffset(noteIndex2, 0, loopInfo.backwardJumpOffsetFromLoopHead()))
> > +        return false;
> > +    // +1 for NOP above.
> 
> You mean JSOP_JUMPTARGET emitted by emitBackwardJump?  If so, and I think
> you do, then say that, not "NOP".

We emit NOP at the beginning of the do-while loop, I think just for adding extra note and telling Ion about do-while structure.
the NOP is removed in bug 1477896.

> js> dis(()=>{do{}while(1)})
> dis(()=>{do{}while(1)})
> flags: LAMBDA ARROW
> loc     op
> -----   --
> main:
> 00000:  nop                             # 
> 
> # from ifne @ 00006
> 00001:  loophead                        # 
> 00002:  loopentry 129                   # 
> 00004:  jumptarget                      # 
> 00005:  true                            # true
> 00006:  ifne 1 (-5)                     # 
> 00011:  jumptarget                      # 
> 00012:  retrval                         # 
> 
> Source notes:
>  ofs line    pc  delta desc     args
> ---- ---- ----- ------ -------- ------
>   0:    2     0 [   0] colspan 9
>   2:    2     0 [   0] while    offset 4
>   4:    2     1 [   1] while    offset 5
>   6:    2     1 [   0] colspan 2
>   8:    2    12 [  11] xdelta  
>   9:    2    12 [   0] colspan 10
> 
> Exception table:
> kind               stack    start      end
>  loop                  0        1       11

the NOP in PC=0 has SRC_WHILE, and also LOOPHEAD in PC=1 has SRC_WHILE.
I'll merge them into SRC_DO_WHILE with 2 slots.

Comment 14

[Tooru Fujisawa [:arai]]

(In reply to Jeff Walden [:Waldo] from comment #12)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +5129,5 @@
> >          return false;
> >      if (!emit1(JSOP_ISNOITER))                            // ITER NEXTITERVAL? ISNOITER
> >          return false;
> >  
> > +    if (!loopInfo.emitBackwardJump(this, JSOP_IFEQ))      // ITER NEXTITERVAL
> 
> emitBackwardJump is a bit too general of a name in light of the loop-centric
> things now being stored, IMO.  What about emitRestartLoop? 
> emitContinueLoop?  emitLoopAgain?  Something along any of these lines, or
> something.

how about emitLoopEnd ?
I'd like to use the same term as loopEndOffsetFromLoopHead you proposed, so that it's clear the where offset is about.

Comment 15

[Jeff Walden [:Waldo]]

emitLoopEnd WFM.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8994403 [details] [diff] [review]
Part 2: Add CForEmitter.

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

::: js/src/frontend/BytecodeEmitter.h
@@ +817,5 @@
>      MOZ_MUST_USE bool emitDo(ParseNode* pn);
>      MOZ_MUST_USE bool emitWhile(ParseNode* pn);
>  
> +    MOZ_MUST_USE bool emitFor(ParseNode* pn, const
> +                              EmitterScope* headLexicalEmitterScope = nullptr);

Nope nope nope on a line break in the middle of a type this way.  :-)

::: js/src/frontend/CForEmitter.h
@@ +120,5 @@
> +    // iteration, just before evaluating the "update" in for(;;) loops.
> +    //
> +    // No freshening occurs in `for (const ...;;)` as there's no point: you
> +    // can't reassign consts. This is observable through the Debugger API. (The
> +    // ES6 spec also skips cloning the environment in this case.)

This comment's bugged me a little since (I think) I wrote it (or most of it), because it puts the cart (the rationale for not-freshening in the spec) before the horse (the spec says not to).  While you're here, or in a rev atop, how about:

"""
ECMAScript doesn't freshen in `for (const ...;;)`.  Lack of freshening isn't directly observable in-language because `const`s can't be mutated, but it *can* be observed in the Debugger API.
"""

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8994404 [details] [diff] [review]
Part 3: Add ForInEmitter.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ -5119,5 @@
> -    loopInfo.setContinueTarget(offset());
> -
> -    // Make sure this code is attributed to the "for".
> -    if (!updateSourceCoordNotes(forInHead->pn_pos.begin))
> -        return false;

These lines do not seem to be present in emitEnd.  What gives?

::: js/src/frontend/ForInEmitter.cpp
@@ +63,5 @@
> +    if (!loopInfo_->emitLoopHead(bce_, Nothing()))    // ITER ITERVAL
> +        return false;
> +
> +    // If the loop had an escaping lexical declaration, replace the current
> +    // environment with an dead zoned one to implement TDZ semantics.

s/replace.*one/reset the declaration's bindings to uninitialized/

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8994408 [details] [diff] [review]
Part 7: Remove unused code/comment for SRC_FOR.

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

Lovely.

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e20a86a9b5756d9bf2ec3c48b1c7ddbdb95a9eba
Bug 1456404 - Part 1: Move loop related bytecode/offset handling to LoopControl. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbce3046bd95e61ced3487cb3d6e5bf4310983f
Bug 1456404 - Part 2: Add CForEmitter. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a44d463eaaca925d705d6145f24d9f18c59a7a
Bug 1456404 - Part 3: Add ForInEmitter. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/b355131844eaba786b4c86479e17f48a96ead435
Bug 1456404 - Part 4: Add ForOfEmitter. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/91e18d8d1e48340f27242342f20f047389af6703
Bug 1456404 - Part 5: Add WhileEmitter. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/61ec9ed5da1aa0a2654ea251223b561cab60073b
Bug 1456404 - Part 6: Add DoWhileEmitter. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/65c4fa236298578b59f3d32893ff69f96793c608
Bug 1456404 - Part 7: Remove unused code/comment for SRC_FOR. r=jwalden

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e20a86a9b575
https://hg.mozilla.org/mozilla-central/rev/6dbce3046bd9
https://hg.mozilla.org/mozilla-central/rev/b4a44d463eaa
https://hg.mozilla.org/mozilla-central/rev/b355131844ea
https://hg.mozilla.org/mozilla-central/rev/91e18d8d1e48
https://hg.mozilla.org/mozilla-central/rev/61ec9ed5da1a
https://hg.mozilla.org/mozilla-central/rev/65c4fa236298