Add helper classes to emit bytecode for loops

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: arai, Assigned: arai)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(7 attachments)

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.
Priority: -- → P2
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
Depends on: 1464311
Depends on: 1477621
Blocks: 1477896
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.
Attachment #8994402 - Flags: review?(jwalden+bmo)
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.
Attachment #8994403 - Flags: review?(jwalden+bmo)
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?
Attachment #8994404 - Flags: review?(jwalden+bmo)
Attachment #8994404 - Flags: feedback?(tcampbell)
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.
Attachment #8994405 - Flags: review?(jwalden+bmo)
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.
Attachment #8994406 - Flags: review?(jwalden+bmo)
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.
Attachment #8994407 - Flags: review?(jwalden+bmo)
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.
Attachment #8994408 - Flags: review?(jwalden+bmo)
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)
Attachment #8994404 - Flags: feedback?(tcampbell)
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".
Attachment #8994402 - Flags: review?(jwalden+bmo) → review+
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.
(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.
emitLoopEnd WFM.
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.
"""
Attachment #8994403 - Flags: review?(jwalden+bmo) → review+
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/
Attachment #8994404 - Flags: review?(jwalden+bmo) → review+
Attachment #8994405 - Flags: review?(jwalden+bmo) → review+
Attachment #8994406 - Flags: review?(jwalden+bmo) → review+
Attachment #8994407 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8994408 [details] [diff] [review]
Part 7: Remove unused code/comment for SRC_FOR.

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

Lovely.
Attachment #8994408 - Flags: review?(jwalden+bmo) → review+
Blocks: 1482003
You need to log in before you can comment on or make changes to this bug.