1308383 - Create a class to handle JumpList and depth in if-then-else in BytecodeEmitter.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Bug 1184922 will add 2 more if-then-else code to BytecodeEmitter.
To emit if-then-else, we need to handle 2 JumpList's, one srcnote, and save/restore stackDepth for then-branch and else-branch.
it would become cleaner if those things are moved into dedicated class.

Comment 1

[Tooru Fujisawa [:arai]]

at first I was thinking a class that does some of those operations in ctor and dtor, but almost all operations are fallible,
so we need to call method in each step.

try is running:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b69f8f63aba

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Cleanup BytecodeEmitter::emitIf.

For now, these patches don't handle TDZCheckCache.


1. Moved setSrcNoteOffset for SRC_IF_ELSE before emitting else part.

2. Removed |bool emittingElse| and |JumpList jmp|, as we can do everything before |goto if_again|

3. merged emitJumpTarget+patchJumpsToTarget to emitJumpTargetAndPatch.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add IfThenElseEmitter.

Added IfThenElseEmitter class, and moved the following things into it:
  * create source note
  * emit JSOP_IFEQ to jump around the then part
  * emit JSOP_GOTO to jump around the else part
  * patch JSOP_IFEQ and JSOP_GOTO
  * save stack depth of the then part, and restore it in the else part

now if-then-else can be done in the following format:

  ... emit the condition here ...

  IfThenElseEmitter ifThenElse(this);
  if (!ifThenElse.emitIfElse())
    return false;

  ... emit the then part here ...

  if (!ifThenElse.emitElse())
    return false;

  ... emit the else part here ...

  if (!ifThenElse.emitEnd())
    return false;

and here's the code for single if-then:

  ... emit the condition here ...

  IfThenElseEmitter ifThenElse(this);
  if (!ifThenElse.emitIfElse())
    return false;

  ... emit the then part here ...

  if (!ifThenElse.emitEnd())
    return false;

and else-if can be chanined:

  ... emit the condition here ...

  IfThenElseEmitter ifThenElse(this);
  if (!ifThenElse.emitIfElse())
    return false;

  ... emit the then part here ...

  if (!ifThenElse.emitElse())
    return false;

  ... emit the condition here ...

  if (!ifThenElse.emitIfElse())
    return false;

  ... emit the then part here ...

  if (!ifThenElse.emitElse())
    return false;

  ... emit the condition here ...

  if (!ifThenElse.emitIfElse())
    return false;

  ... emit the then part here ...

  if (!ifThenElse.emitElse())
    return false;

  ... emit the else part here ...

  if (!ifThenElse.emitEnd())
    return false;

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Cleanup BytecodeEmitter::emitConditionalExpression.

BytecodeEmitter::emitConditionalExpression uses same format, so moved setSrcNoteOffset just like Part 1.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Use IfThenElseEmitter in BytecodeEmitter::emitConditionalExpression.

Added IfThenElseEmitter::emitCond to support SRC_COND, and used it in BytecodeEmitter::emitConditionalExpression.

  ... emit the condition here ...

  IfThenElseEmitter ifThenElse(this);
  if (!ifThenElse.emitCond())
    return false;

  ... emit the then part here ...

  if (!ifThenElse.emitElse())
    return false;

  ... emit the else part here ...

  if (!ifThenElse.emitEnd())
    return false;

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8801510 [details] [diff] [review]
Part 1: Cleanup BytecodeEmitter::emitIf.

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

Looks like a nice simplification.

::: js/src/frontend/BytecodeEmitter.cpp
@@ -5667,5 @@
> -         */
> -        emittingElse = false;
> -        if (!setSrcNoteOffset(noteIndex, 0, jmp.offset - beq.offset))
> -            return false;
> -    }

Good refactor. So we had two different codepaths for setSrcNoteOffset around if-elses, one for if-else-if chains, and one for if-else?

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8801512 [details] [diff] [review]
Part 2: Add IfThenElseEmitter.

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

Looks great.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4512,5 @@
>          return false;
>      return true;
>  }
>  
> +class IfThenElseEmitter

Annotate this with MOZ_STACK_CLASS.

@@ +4515,5 @@
>  
> +class IfThenElseEmitter
> +{
> +    BytecodeEmitter* bce_;
> +    JumpList jumpsAroundThen_;

This is misnamed. It's always a single jump around the then part, right? It gets re-set as a blank JumpList for each if emitted in the chain. It's only the jumpsAroundElse is a chain of more than one jump.

Please rename this to jumpAroundThen.

@@ +4519,5 @@
> +    JumpList jumpsAroundThen_;
> +    JumpList jumpsAroundElse_;
> +    unsigned noteIndex_;
> +    int32_t thenDepth_;
> +    using State = enum {

Since this enum is anonymous anyways, how about just

enum {
  Start,
  ...
} state_;

@@ +4541,5 @@
> +    {}
> +
> +  private:
> +    bool
> +    emitIf(State nextState) {

Style nit, here and below: for inline method defns I've always seen return type on the same line.

@@ +5747,1 @@
>      ParseNode* pn3 = pn->pn_kid3;

While you're here, please rename pn3 to something more informative like elseNode.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8801512 [details] [diff] [review]
Part 2: Add IfThenElseEmitter.

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

Oh my bad, the State enum isn't anonymous. I'd prefer |enum State| over using State. Up to you, I'm fine with either.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8801516 [details] [diff] [review]
Part 4: Use IfThenElseEmitter in BytecodeEmitter::emitConditionalExpression.

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

I wonder if you might have stricter stack depth asserts for the Cond case, because each branch will only push exactly one value.

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

(In reply to Shu-yu Guo [:shu] from comment #6)
> So we had two different codepaths for setSrcNoteOffset around
> if-elses, one for if-else-if chains, and one for if-else?

Yes :)


(In reply to Shu-yu Guo [:shu] from comment #9)
> I wonder if you might have stricter stack depth asserts for the Cond case,
> because each branch will only push exactly one value.

It would be nice to have some assertion function that checks all branches pushed the same number of values, maybe to call after emitEnd.
Will try implementing it.

Comment 11

[Tooru Fujisawa [:arai]]

about TDZCheckCachek, it might be better avoid handling it unless we have similar helper class for all branches and loops.
as emitting conditional branches with emitTree and emitConditionallyExecutedTree on different places might be confusing.
So, I'll leave it to other bug, is it okay?

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Check the number of pushed or popped values on each branch in IfThenElseEmitter.

Now IfThenElseEmitter checks that the number of values pushed by each branches are same.
Also, caller can check the number after emitEnd by `ifThenElse.pushed()`.

at first I was about to add a function that asserts, like `ifThenElse.assertPushed(1)`.
but it doesn't show the expected number in error message.
so changed to `MOZ_ASSERT(ifThenElse.pushed() == -1);`

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbd5f2898b4f

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> about TDZCheckCachek, it might be better avoid handling it unless we have
> similar helper class for all branches and loops.
> as emitting conditional branches with emitTree and
> emitConditionallyExecutedTree on different places might be confusing.
> So, I'll leave it to other bug, is it okay?

Sure thing.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8802797 [details] [diff] [review]
Part 5: Check the number of pushed or popped values on each branch in IfThenElseEmitter.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4782,5 @@
>  
>              if (!isHead) {
>                  if (!ifThenElse.emitEnd())
>                      return false;
> +                MOZ_ASSERT(ifThenElse.pushed() == -1);

What does -1 here mean, that I popped a value in the 'then' part?

Comment 15

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Shu-yu Guo [:shu] from comment #14)
> Comment on attachment 8802797 [details] [diff] [review]
> Part 5: Check the number of pushed or popped values on each branch in
> IfThenElseEmitter.
> 
> Review of attachment 8802797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4782,5 @@
> >  
> >              if (!isHead) {
> >                  if (!ifThenElse.emitEnd())
> >                      return false;
> > +                MOZ_ASSERT(ifThenElse.pushed() == -1);
> 
> What does -1 here mean, that I popped a value in the 'then' part?

yes, that block pops a value.
maybe we should add popped() method, so that it's clear what it means.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Add IfThenElseEmitter::popped.

changed ifThenElse.pushed() == -1 to ifThenElse.popped() == 1

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8805812 [details] [diff] [review]
Part 6: Add IfThenElseEmitter::popped.

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

:)

Comment 18

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b419577e090926d77b7ccc670a12af662e10c80b
Bug 1308383 - Part 1: Cleanup BytecodeEmitter::emitIf. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/25fe60b5746b4a6ab6bcdea940c1520980915d36
Bug 1308383 - Part 2: Add IfThenElseEmitter. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8649b3008e1bd588b48156a5496c7b761be311a
Bug 1308383 - Part 3: Cleanup BytecodeEmitter::emitConditionalExpression. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f884840b536fb4ce6d09ccfddec74d2b3a9d314
Bug 1308383 - Part 4: Use IfThenElseEmitter in BytecodeEmitter::emitConditionalExpression. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/83472f5679d4c349c01b546c50cf1ca8ea004ee8
Bug 1308383 - Part 5: Check the number of pushed or popped values on each branch in IfThenElseEmitter. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/4561836d44dcadefc813936a194fb42567a0ab2b
Bug 1308383 - Part 6: Add IfThenElseEmitter::popped. r=shu

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b419577e0909
https://hg.mozilla.org/mozilla-central/rev/25fe60b5746b
https://hg.mozilla.org/mozilla-central/rev/f8649b3008e1
https://hg.mozilla.org/mozilla-central/rev/5f884840b536
https://hg.mozilla.org/mozilla-central/rev/83472f5679d4
https://hg.mozilla.org/mozilla-central/rev/4561836d44dc