Closed Bug 1308383 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(6 files)

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.
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
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
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.
Attachment #8801510 - Flags: review?(shu)
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;
Attachment #8801512 - Flags: review?(shu)
BytecodeEmitter::emitConditionalExpression uses same format, so moved setSrcNoteOffset just like Part 1.
Attachment #8801515 - Flags: review?(shu)
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;
Attachment #8801516 - Flags: review?(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?
Attachment #8801510 - Flags: review?(shu) → review+
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.
Attachment #8801512 - Flags: review?(shu) → review+
Attachment #8801515 - Flags: review?(shu) → review+
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.
Attachment #8801516 - Flags: review?(shu) → review+
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.
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.
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?
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
Attachment #8802797 - Flags: review?(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 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?
Attachment #8802797 - Flags: review?(shu) → review+
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.
changed ifThenElse.pushed() == -1 to ifThenElse.popped() == 1
Attachment #8805812 - Flags: review?(shu)
Comment on attachment 8805812 [details] [diff] [review]
Part 6: Add IfThenElseEmitter::popped.

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

:)
Attachment #8805812 - Flags: review?(shu) → review+
See Also: → 1322069
See Also: → 1322076
You need to log in before you can comment on or make changes to this bug.