1466000 - Add helper classes to emit bytecode for CallExpression and NewExpression.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

Just like others (bug 1456006, bug 1456404).

To emit CallExpression, we need to handle special callee, which includes name, property, element, and some others.
I'm going to add:
  * PropOpEmitter: emit property operation (get/set/delete/inc/dec/assignment)
  * ElemOpEmitter: emit element operation (same as above)
  * NameOpEmitter: emit name operation (get/set/inc/dec/assignment)
  * CallOrNewEmitter: emit call and new, using classes above

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Emit callee and this at once.

10 patches total.
6 patches for preparation, and 4 patches for each class.
most of the preparation parts will be moved to the newly added classes later.

[Part 1]

When calling a function, `this` value should be pushed onto `callee`, and in some case `this` value comes from `callee` expression, and in some case undefined.
all those handling should be moved to CallOrNewEmitter later, but first merging `this` handling info `callee` handling.

currently BytecodeEmitter::emitCallee pushes `callee` value, and also `this` value if it's not undefined, and modifies out-param callop which controls whether to push undefined.
moved `this` part there and renamed the method to BytecodeEmitter::emitCalleeAndThis, which always pushes `this` part, even if it's undefined.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move call context handling from BytecodeEmitter::emitGetNameAtLocation to BytecodeEmitter::emitCalleeAndThis.

[Part 2]

`callContext` parameter of BytecodeEmitter::emitGetNameAtLocation becomes true only when called from BytecodeEmitter::emitCalleeAndThis, and it's indeed specific to call.
Moved the handling into BytecodeEmitter::emitCalleeAndThis.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add stack comments for prop and elem related methods in BytecodeEmitter.

[Part 3]

Mostly just added stack comments.
In some case modified the branches for simplicity and reduced the number of branches.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Remove emitAtopOp with NameNode*.

[Part 4]

BytecodeEmitter::emitAtomOp with NameNode is a thin-wrapper for BytecodeEmitter::emitAtomOp with JSAtom*.
Just removed it and made callers to call the one with JSAtom*.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Pass JSOp to BytecodeEmitter::emitAssignment.

[Part 5]

BytecodeEmitter::emitAssignment receives ParseNodeKind and immediately converts it to JSOp.
Moved the conversion to caller side and eliminated the call for simple assignment.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Add BytecodeEmitter::emitGetFunctionThis variant with offset.

[Part 6]

BytecodeEmitter::emitGetFunctionThis receives ParseNode and calls BytecodeEmitter::emitTree, which is effectively same as updateLineNumberNotes + emitGetName(cx->names().dotThis).

Added variants for BytecodeEmitter::emitGetFunctionThis which receives the offset, to use it without ParseNode.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Add PropOpEmitter.

[Part 7]

PropOpEmitter handles property operation `expr.key`, with get/set/delete/inc/dec/assignment.

the usage is in the comment above the class declaration.

list of some correspondance:
  * BytecodeEmitter::emitSuperPropLHS call
    -> BytecodeEmitter::emitGetThisForSuperBase call with PropOpEmitter method call
  * BytecodeEmitter::emitPropOp
    -> PropOpEmitter method call
  * BytecodeEmitter::emitSuperGetProp
    -> PropOpEmitter::emitGet

  * the content of BytecodeEmitter::emitPropIncDec
    -> PropOpEmitter::{emitObj,emitIncDec}
  * the property part of BytecodeEmitter::emitDestructuringLHSRef
    -> PropOpEmitter::{emitObj,emitRhs}
  * the property part of BytecodeEmitter::emitSetOrInitializeDestructuring
    -> PropOpEmitter::{skipObjAndRhs,emitAssignment}
  * the property part of BytecodeEmitter::emitAssignment
    -> PropOpEmitter::{emitObj,emitGet,emitRhs,emitAssignment}
  * the content of BytecodeEmitter::emitDeleteProperty
    -> PropOpEmitter::{emitObj,emitDelete}
  * property get/call
    -> PropOpEmitter::{emitObj,emitGet}


Also added a variant of BytecodeEmitter::emitAtomOp with atomIndex.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Add ElemOpEmitter.

[Part 8]

ElemOpEmitter handles element operation `expr[key]`, with get/set/delete/inc/dec/assignment,
just like PropOpEmitter.

the usage is in the comment above the class declaration.

list of some correspondance:
  * BytecodeEmitter::{emitElemOperands,emitElemOp}
    -> BytecodeEmitter::emitElemObjAndKey and ElemOpEmitter methods
  * BytecodeEmitter::{emitSuperElemOperands,emitSuperGetElem}
    -> BytecodeEmitter::emitElemObjAndKey and ElemOpEmitter methods

  * the contet of BytecodeEmitter::emitElemIncDec
    -> BytecodeEmitter::emitElemObjAndKey call and ElemOpEmitter::emitIncDec
  * the element part of BytecodeEmitter::emitDestructuringLHSRef
    -> BytecodeEmitter::emitElemObjAndKey call and ElemOpEmitter::emitRhs
  * the element part of BytecodeEmitter::emitSetOrInitializeDestructuring
    -> ElemOpEmitter::{skipObjAndKeyAndRhs,emitAssignment}
  * the element part of BytecodeEmitter::emitAssignment
    -> BytecodeEmitter::emitElemObjAndKey call and ElemOpEmitter::{emitGet,emitRhs,emitAssignment}
  * the contet of BytecodeEmitter::emitDeleteElement
    -> BytecodeEmitter::emitElemObjAndKey call or ElemOpEmitter::{emitObj,emitKey}, and ElemOpEmitter::emitDelete
  * element get/call
    -> BytecodeEmitter::emitElemObjAndKey call and ElemOpEmitter::emitGet

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Add NameOpEmitter.

[Part 9]

NameOpEmitter handles name (variable) operation, with get/set/inc/dec/assignment,
just like others.

the usage is in the comment above the class declaration.

list of some correspondance:
  * the content of BytecodeEmitter::emitGetNameAtLocation
    -> NameOpEmitter::emitGet

  * BytecodeEmitter::emitSetOrInitializeNameAtLocation
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * BytecodeEmitter::emitGetNameAtLocationForCompoundAssignment
    -> NameOpEmitter::emitRhs

  * the content of BytecodeEmitter::emitNameIncDec
    -> NameOpEmitter::emitIncDec
  * the name part of BytecodeEmitter::emitSetThis
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * the name part of BytecodeEmitter::emitSetOrInitializeDestructuring
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * the content of BytecodeEmitter::emitSingleDeclaration
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * the name part of BytecodeEmitter::emitAssignment
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * the name part of BytecodeEmitter::emitInitializeForInOrOfTarget
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * the for head with initializer in BytecodeEmitter::emitForIn
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * function declaration in BytecodeEmitter::emitFunction
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * name get/call
    -> NameOpEmitter::emitGet
  * special binding (.this, .generator)
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * parameters in BytecodeEmitter::emitFunctionFormalParam
    -> NameOpEmitter::{emitRhs,emitAssignment}
  * the content of BytecodeEmitter::emitLexicalInitialization
    -> NameOpEmitter::{emitRhs,emitAssignment}

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Add CallOrNewEmitter.

[Part 10]

CallOrNewEmitter handles call and new expressions, including `callee`, `this` handling, parameter, and optimization for spread,
using the classes above.

the usage is in the comment above the class declaration.

BytecodeEmitter::emitCalleeAndThis now uses CallOrNewEmitter.
CallOrNewEmitter returns PropOpEmitter/ElemOpEmitter instances for those cases.

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 9011683 [details] [diff] [review]
Part 1: Emit callee and this at once.

as talked in IRC, redirecting the review

Comment 12

[Eric Faust [:efaust]]

Comment on attachment 9011683 [details] [diff] [review]
Part 1: Emit callee and this at once.

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

Looks good.

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 9011684 [details] [diff] [review]
Part 2: Move call context handling from BytecodeEmitter::emitGetNameAtLocation to BytecodeEmitter::emitCalleeAndThis.

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

Feel free to ignore the note. I can see it going either way.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7171,5 @@
>  BytecodeEmitter::emitCalleeAndThis(ParseNode* callee, ParseNode* call, bool isCall, bool isNew)
>  {
>      bool needsThis = !isCall;
>      switch (callee->getKind()) {
> +      case ParseNodeKind::Name: {

I kind of dislike how bulky this code is, and it feels like it at least the |isCall| branch wants to be a helper function, but I don't like anything I might call it, since it's going to be so single-user

emitThisForNameLocation(), maybe?

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 9011685 [details] [diff] [review]
Part 3: Add stack comments for prop and elem related methods in BytecodeEmitter.

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

\o/ Nice! r=me with code structure change factored to part 2

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4480,5 @@
>                  }
>                  elemOp = JSOP_GETELEM;
>              }
> +            if (!emitElemOpBase(elemOp)) {                // [Super]
> +                //                                        // THIS KEY OBJ ELEM

do we need the first set of slashes?

@@ +7265,2 @@
>              if (isCall) {
> +                if (!emitElemOp(elem, JSOP_CALLELEM)) {   // THIS CALLEE

Ideally, this change would be rolled with part 2, and this diff would be comment-only, right?

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 9011686 [details] [diff] [review]
Part 4: Remove emitAtopOp with NameNode*.

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

lgtm. r=me with comment addressed

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2061,5 @@
>      if (!emitPropLHS(prop)) {
>          return false;
>      }
>  
> +    JSAtom* atom = prop->key().atom();

Can we move this down next to its use on line 2070?

@@ +2080,5 @@
>  
>  bool
>  BytecodeEmitter::emitSuperGetProp(PropertyAccess* prop, bool isCall)
>  {
> +    JSAtom* atom = prop->key().atom();

Same here.

@@ +2107,5 @@
>      bool post;
>      bool isSuper = prop->isSuper();
>      JSOp binop = GetIncDecInfo(incDec->getKind(), &post);
>  
> +    JSAtom* atom = prop->key().atom();

Am I missing something? Do we want this hoisted later in the stack, or something?

Comment 16

[Eric Faust [:efaust]]

Comment on attachment 9011687 [details] [diff] [review]
Part 5: Pass JSOp to BytecodeEmitter::emitAssignment.

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

Yeah, that passing of a random PNK to get the desired behavior is pretty ugly. Nice fix, but it needs a little bit more for later readability.

r=me with variable name changed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4324,5 @@
>      }
>  }
>  
>  bool
> +BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs)

I understand why it's called |op| for ease of refactoring, but that name doesn't do me any good as a consumer of this function, to know what the op does. I am forced to go read the code to understand that it's a compound op.

Please rename it to |compoundOp|. Bonus points if we stop open-checking against (op == JSOP_NOP) everywhere in this function and switch to a |bool isCompound = compoundOp != JSOP_NOP| at the top.

Comment 17

[Eric Faust [:efaust]]

Comment on attachment 9011688 [details] [diff] [review]
Part 6: Add BytecodeEmitter::emitGetFunctionThis variant with offset.

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

r=me with question addressed

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6059,5 @@
> +    return emitGetFunctionThis(Some(pn->pn_pos.begin));
> +}
> +
> +bool
> +BytecodeEmitter::emitGetFunctionThis(mozilla::Maybe<uint32_t> offset)

prefer const reference.

@@ +6061,5 @@
> +
> +bool
> +BytecodeEmitter::emitGetFunctionThis(mozilla::Maybe<uint32_t> offset)
> +{
> +    if (offset) {

I would feel a little safer about this if there was an assert:

Something like factoring ParseNodeRequiresSpecialLineNumberNotes to take a PNK, and then asserting that PNK::Name isn't special.

What do you think? Seems extra safe, but maybe too paranoid.

Comment 18

[Eric Faust [:efaust]]

Comment on attachment 9011689 [details] [diff] [review]
Part 7: Add PropOpEmitter.

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

\o/ this is sweet! I feel like It's a LOT clearer what's going on, and in general I look forward to the factored future with more of these helpers.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3066,5 @@
>              MOZ_CRASH("emitSetOrInitializeDestructuring: bad lhs kind");
>          }
>  
>          // Pop the assigned value.
> +        if (!emit1(JSOP_POP)) {                           //

oops. :P

@@ +6774,5 @@
> +        UnaryNode* base = &propExpr->expression().as<UnaryNode>();
> +        if (!emitGetThisForSuperBase(base)) {             // THIS
> +            return false;
> +        }
> +        if (!poe.emitDelete(propExpr->key().atom())) {    // THIS

Is there an advantage to having another call in the super branch?

I would expect:
if (isSuper) { 
   ...
} else {
   ...
}
return poe.emitDelete

similarly to what's done in other places.

::: js/src/frontend/PropOpEmitter.cpp
@@ +100,5 @@
> +            return false;
> +        }
> +    }
> +
> +#ifdef DEBUG

out of curiosity, can you put a DebugOnly in another struct?

Comment 19

[Eric Faust [:efaust]]

Comment on attachment 9011690 [details] [diff] [review]
Part 8: Add ElemOpEmitter.

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

Very nice. LGTM, apart from the one function name. r=me with that addressed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6720,5 @@
> +        }
> +        if (!eoe.emitKey()) {                             // THIS
> +            return false;
> +        }
> +        if (!emitTree(&elemExpr->key())) {                // THIS KEY

Isn't this what emitKey() does?

EDIT: I see. See comment below.

@@ +6732,5 @@
> +
> +    if (!emitElemObjAndKey(elemExpr, false, eoe)) {       // OBJ KEY
> +        return false;
> +    }
> +    if (!eoe.emitDelete()) {                               // SUCCEEDED

Similarly to the previous delete, I would expect this to converge, as the gets do.

::: js/src/frontend/ElemOpEmitter.cpp
@@ +30,5 @@
> +    return true;
> +}
> +
> +bool
> +ElemOpEmitter::emitKey()

This function needs a new name, for what it does. It doesn't emit the key at all! It emits situational lhs modifiers.

prepareForKey(), perhaps?

Comment 20

[Eric Faust [:efaust]]

Comment on attachment 9011691 [details] [diff] [review]
Part 9: Add NameOpEmitter.

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

it seems to me that emitRhs should be called emitLhs. I don't feel comfortable continuing until I understand what I don't understand.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3795,5 @@
> +        // initializer.
> +        MOZ_ASSERT(declList->isKind(ParseNodeKind::Let),
> +                   "var declarations without initializers handled above, "
> +                   "and const declarations must have initializers");
> +        Unused << declList; // silence clang -Wunused-lambda-capture in opt builds

OK, but we aren't in a lambda anymore, it seems like.

::: js/src/frontend/NameOpEmitter.cpp
@@ +126,5 @@
> +    return true;
> +}
> +
> +bool
> +NameOpEmitter::emitRhs()

OK, I must be missing something. This function doesn't emit the initializer at all. we have emitInitializer and emitAssignment for that. This like emits the LEFT-hand side binding, right?

::: js/src/frontend/NameOpEmitter.h
@@ +51,5 @@
> +//     emit_add_op_here();
> +//     noe.emitAssignment();
> +//
> +//   `name = 10;`
> +//   of `let name = 10;`

is this meant to be "or" or "of"?

Comment 21

[Eric Faust [:efaust]]

Comment on attachment 9011692 [details] [diff] [review]
Part 10: Add CallOrNewEmitter.

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

In general, this looks good, but we still have a naming issue we should talk about. Maybe it's best to handle this on IRC and report back.

Comment 22

[Eric Faust [:efaust]]

In general, the Emitters are great! I think the API names need to be more genuine about how they expect to play back and forth between the caller and their work.

Stuff that's actually emitting operations should be called emit${State}
Stuff that's shuffling the state machine along and operating on the results of the last state, but needs the user to push some value (like a key, or a RHS tree), should be called prepareFor${NEXT_STATE} where it's clear you're going to want to call it, and also what order your stuff should go in with respect to its.

This commentary also applies to {Prop,Elem}OpEmitter as well.

Thoughts on this?

Comment 23

[Tooru Fujisawa [:arai]]

thank you for reviewing :D

the basic rule I was using is:
  * if there's anything to emit outside of the helper:
      helper.emitFoo(...);
      emit(foo);
  * if helper emits everything:
      helper.emitFoo(...);
  * at the end of using the helper:
      helper.emitEnd(...);

I haven't thought much about if "emit" fits, so I'm open to change the method names :)
and yeah, "prepareFor" sounds reasonable, and that will clarify what the caller should do.
I'll modify those method names, and file a bug to fix the existing other helper classes.


(In reply to Eric Faust [:efaust] from comment #14)
> Comment on attachment 9011685 [details] [diff] [review]
> Part 3: Add stack comments for prop and elem related methods in
> BytecodeEmitter.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4480,5 @@
> >                  }
> >                  elemOp = JSOP_GETELEM;
> >              }
> > +            if (!emitElemOpBase(elemOp)) {                // [Super]
> > +                //                                        // THIS KEY OBJ ELEM
> 
> do we need the first set of slashes?

it's there just to help auto-indentation.
otherwise the 2nd "//" is moved to the place of the 1st "//"
(maybe I just have to modify my emacs style definition tho...)


> @@ +7265,2 @@
> >              if (isCall) {
> > +                if (!emitElemOp(elem, JSOP_CALLELEM)) {   // THIS CALLEE
> 
> Ideally, this change would be rolled with part 2, and this diff would be
> comment-only, right?

I modified the code there just because otherwise the stack state is hard to follow.
I'll put the code modification in other patch.


(In reply to Eric Faust [:efaust] from comment #17)
> Comment on attachment 9011688 [details] [diff] [review]
> Part 6: Add BytecodeEmitter::emitGetFunctionThis variant with offset.
> 
> @@ +6061,5 @@
> > +
> > +bool
> > +BytecodeEmitter::emitGetFunctionThis(mozilla::Maybe<uint32_t> offset)
> > +{
> > +    if (offset) {
> 
> I would feel a little safer about this if there was an assert:
> 
> Something like factoring ParseNodeRequiresSpecialLineNumberNotes to take a
> PNK, and then asserting that PNK::Name isn't special.
> 
> What do you think? Seems extra safe, but maybe too paranoid.

the `Maybe<uint32_t>` variant is added in order to emit code for `this`, without having ParseNode.  so at least we shouldn't touch PNK in this function.
(eventually I'm going to split BytecodeEmitter into 2 parts, core part and wrapper for ParseNode* tree)

If you mean adding the call to `ParseNode*` variant, I'll do.


(In reply to Eric Faust [:efaust] from comment #18)
> Comment on attachment 9011689 [details] [diff] [review]
> Part 7: Add PropOpEmitter.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +3066,5 @@
> >              MOZ_CRASH("emitSetOrInitializeDestructuring: bad lhs kind");
> >          }
> >  
> >          // Pop the assigned value.
> > +        if (!emit1(JSOP_POP)) {                           //
> 
> oops. :P

If you mean "//", it's there to clarify there's nothing on the stack (only for this operation's scope.  there can be more values under there for loop etc)


> ::: js/src/frontend/PropOpEmitter.cpp
> @@ +100,5 @@
> > +            return false;
> > +        }
> > +    }
> > +
> > +#ifdef DEBUG
> 
> out of curiosity, can you put a DebugOnly in another struct?

DebugOnly consumes a space even on non-debug, and these classes can be used with Maybe<> (which I believe compiler less likely be able to optimize away the space),
so I'd like to use macro.
https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/mfbt/DebugOnly.h#32-33


(In reply to Eric Faust [:efaust] from comment #20)
> Comment on attachment 9011691 [details] [diff] [review]
> Part 9: Add NameOpEmitter.
> 
> ::: js/src/frontend/NameOpEmitter.h
> @@ +51,5 @@
> > +//     emit_add_op_here();
> > +//     noe.emitAssignment();
> > +//
> > +//   `name = 10;`
> > +//   of `let name = 10;`
> 
> is this meant to be "or" or "of"?

I meant, "`name = 10;` part of `let name = 10;`".
I'll fix the comment

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Add PropOpEmitter. r=efaust

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Add ElemOpEmitter. r=efaust

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Add NameOpEmitter.

Fixed the method names

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Add CallOrNewEmitter.

Comment 28

[Eric Faust [:efaust]]

(In reply to Tooru Fujisawa [:arai] from comment #23)
> (In reply to Eric Faust [:efaust] from comment #14)
> > Comment on attachment 9011685 [details] [diff] [review]
> > Part 3: Add stack comments for prop and elem related methods in
> > BytecodeEmitter.
> > 
> > ::: js/src/frontend/BytecodeEmitter.cpp
> > @@ +4480,5 @@
> > >                  }
> > >                  elemOp = JSOP_GETELEM;
> > >              }
> > > +            if (!emitElemOpBase(elemOp)) {                // [Super]
> > > +                //                                        // THIS KEY OBJ ELEM
> > 
> > do we need the first set of slashes?
> 
> it's there just to help auto-indentation.
> otherwise the 2nd "//" is moved to the place of the 1st "//"
> (maybe I just have to modify my emacs style definition tho...)
> 

Ok, sounds good. I found a few other places where reading the code I was grateful for them.

> 
> > @@ +7265,2 @@
> > >              if (isCall) {
> > > +                if (!emitElemOp(elem, JSOP_CALLELEM)) {   // THIS CALLEE
> > 
> > Ideally, this change would be rolled with part 2, and this diff would be
> > comment-only, right?
> 
> I modified the code there just because otherwise the stack state is hard to
> follow.
> I'll put the code modification in other patch.
> 
> 
> (In reply to Eric Faust [:efaust] from comment #17)
> > Comment on attachment 9011688 [details] [diff] [review]
> > Part 6: Add BytecodeEmitter::emitGetFunctionThis variant with offset.
> > 
> > @@ +6061,5 @@
> > > +
> > > +bool
> > > +BytecodeEmitter::emitGetFunctionThis(mozilla::Maybe<uint32_t> offset)
> > > +{
> > > +    if (offset) {
> > 
> > I would feel a little safer about this if there was an assert:
> > 
> > Something like factoring ParseNodeRequiresSpecialLineNumberNotes to take a
> > PNK, and then asserting that PNK::Name isn't special.
> > 
> > What do you think? Seems extra safe, but maybe too paranoid.
> 
> the `Maybe<uint32_t>` variant is added in order to emit code for `this`,
> without having ParseNode.  so at least we shouldn't touch PNK in this
> function.
> (eventually I'm going to split BytecodeEmitter into 2 parts, core part and
> wrapper for ParseNode* tree)
> 
> If you mean adding the call to `ParseNode*` variant, I'll do.
> 
> 
> (In reply to Eric Faust [:efaust] from comment #18)
> > Comment on attachment 9011689 [details] [diff] [review]
> > Part 7: Add PropOpEmitter.
> > 
> > ::: js/src/frontend/BytecodeEmitter.cpp
> > @@ +3066,5 @@
> > >              MOZ_CRASH("emitSetOrInitializeDestructuring: bad lhs kind");
> > >          }
> > >  
> > >          // Pop the assigned value.
> > > +        if (!emit1(JSOP_POP)) {                           //
> > 
> > oops. :P
> 
> If you mean "//", it's there to clarify there's nothing on the stack (only
> for this operation's scope.  there can be more values under there for loop
> etc)
> 

Can we make that a little more explicit, then? Some !STACK EMPTY! or something? Otherwise, it still reads like a comment floating in space to me.

> 
> > ::: js/src/frontend/PropOpEmitter.cpp
> > @@ +100,5 @@
> > > +            return false;
> > > +        }
> > > +    }
> > > +
> > > +#ifdef DEBUG
> > 
> > out of curiosity, can you put a DebugOnly in another struct?
> 
> DebugOnly consumes a space even on non-debug, and these classes can be used
> with Maybe<> (which I believe compiler less likely be able to optimize away
> the space),
> so I'd like to use macro.
> https://searchfox.org/mozilla-central/rev/
> 924e3d96d81a40d2f0eec1db5f74fc6594337128/mfbt/DebugOnly.h#32-33
> 

Yep. My bad.
> 
> (In reply to Eric Faust [:efaust] from comment #20)
> > Comment on attachment 9011691 [details] [diff] [review]
> > Part 9: Add NameOpEmitter.
> > 
> > ::: js/src/frontend/NameOpEmitter.h
> > @@ +51,5 @@
> > > +//     emit_add_op_here();
> > > +//     noe.emitAssignment();
> > > +//
> > > +//   `name = 10;`
> > > +//   of `let name = 10;`
> > 
> > is this meant to be "or" or "of"?
> 
> I meant, "`name = 10;` part of `let name = 10;`".
> I'll fix the comment

Thanks.

Comment 29

[Eric Faust [:efaust]]

Comment on attachment 9014676 [details] [diff] [review]
Part 9: Add NameOpEmitter.

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

This reads MUCH cleaner to me. Thanks for changing the factoring slightly. Nice work!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2237,5 @@
>          lexicalLoc = loc;
>      }
>  
> +    NameOpEmitter noe(this, name, lexicalLoc, NameOpEmitter::Kind::Initialize);
> +    if (!noe.prepareForRhs()) {                           //

:)

Comment 30

[Eric Faust [:efaust]]

Comment on attachment 9014677 [details] [diff] [review]
Part 10: Add CallOrNewEmitter.

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

OK, this looks good, but it seems like maybe the wrong patch was uploaded? r=me, but please do take a look at the comments.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6812,5 @@
>        case ParseNodeKind::Elem: {
>          MOZ_ASSERT(emitterMode != BytecodeEmitter::SelfHosting);
>          PropertyByValue* elem = &callee->as<PropertyByValue>();
>          bool isSuper = elem->isSuper();
> +        ElemOpEmitter& eoe = cone.emitElemCallee(isSuper);

prepare..

For readability, it would also be nice to have a blank line above this, also. This local variable is actually doing emission work, rather than just extracting some temps.

@@ +6826,5 @@
>  
>          break;
>        }
>        case ParseNodeKind::Function:
> +        if (!cone.emitFunctionCallee()) {

prepareForFunctionCallee()

@@ +6841,5 @@
>              return false;
>          }
>          break;
>        default:
> +        if (!cone.emitOtherCallee()) {

Seems like this also wants to be prepareForOtherCallee, no?

@@ +6899,5 @@
> +        reportError(argsList, isCall ? JSMSG_TOO_MANY_FUN_ARGS : JSMSG_TOO_MANY_CON_ARGS);
> +        return false;
> +    }
> +    if (!isSpread) {
> +        if (!cone.emitArguments()) {                      // CALLEE THIS

prepareForNonSpreadArguments()?

@@ +6914,5 @@
> +            if (!emitTree(spreadNode->kid())) {           // CALLEE THIS ARG0
> +                return false;
> +            }
> +        }
> +        if (!cone.emitSpreadArguments()) {                // CALLEE THIS

emitSpreadArgumentsTest(), maybe?

Comment 31

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed41dadd8cff2a4f9585dc250a6fcf1995a193c
Bug 1466000 - Part 1: Emit callee and this at once. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/1674f2fff9a61718e6a198a644d279f9aa06f670
Bug 1466000 - Part 2: Move call context handling from BytecodeEmitter::emitGetNameAtLocation to BytecodeEmitter::emitCalleeAndThis. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c3a4c98e406a6b1c75f3e2cd195f4a01ad2d47
Bug 1466000 - Part 3: Add stack comments for prop and elem related methods in BytecodeEmitter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a968c6b847b250464c7b069ab29f640ce2148c3
Bug 1466000 - Part 4: Remove emitAtopOp with NameNode*. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/31026dec4c105a910a5838bfd38041801ba40c33
Bug 1466000 - Part 5: Pass JSOp to BytecodeEmitter::emitAssignment. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/cad2c1babaf228066ced4fd7c94017026a1d482c
Bug 1466000 - Part 6: Add BytecodeEmitter::emitGetFunctionThis variant with offset. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/e29ced1ead38118a28a170f99975936caafae178
Bug 1466000 - Part 7: Add PropOpEmitter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f8a1e5fa5d4ecb83ff2c82e24831550117c5e3
Bug 1466000 - Part 8: Add ElemOpEmitter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5da918f6d7b1e650b56b96570648628ee92b1a
Bug 1466000 - Part 9: Add NameOpEmitter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f55976a9e9115c9f41075843bc48955684364d9
Bug 1466000 - Part 10: Add CallOrNewEmitter. r=efaust

Comment 32

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/0ed41dadd8cf
https://hg.mozilla.org/mozilla-central/rev/1674f2fff9a6
https://hg.mozilla.org/mozilla-central/rev/b1c3a4c98e40
https://hg.mozilla.org/mozilla-central/rev/3a968c6b847b
https://hg.mozilla.org/mozilla-central/rev/31026dec4c10
https://hg.mozilla.org/mozilla-central/rev/cad2c1babaf2
https://hg.mozilla.org/mozilla-central/rev/e29ced1ead38
https://hg.mozilla.org/mozilla-central/rev/61f8a1e5fa5d
https://hg.mozilla.org/mozilla-central/rev/1a5da918f6d7
https://hg.mozilla.org/mozilla-central/rev/4f55976a9e91