[Interpreter] Add for/while/do support to the bytecode generator.

[Interpreter] Add for/while/do support to the bytecode generator.

Improve bytecode generation for if when there's no else clause.

Display target addresses for jump instructions in
Bytecode::Disassemble().

BUG=v8:4280
LOG=N

Committed: https://crrev.com/a7e16e513241f6d62f35e763c14aff5ca445195e
Cr-Commit-Position: refs/heads/master@{#31052}

[oth, 2015-09-30 12:57:02]

oth@chromium.org changed reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[oth, 2015-09-30 12:57:03]

mstarzinger@chromium.org: Please review changes in interpreter.

rmcilroy@chromium.org: Please review all.

[Michael Starzinger, 2015-09-30 13:27:36]

https://codereview.chromium.org/1373903005/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:28: virtual ~ControlScope() {}
Scope popping seems to be missing from destructor.

[oth, 2015-09-30 15:54:46]

Thanks Michael, code fixed and added a test for nested loops.

https://codereview.chromium.org/1373903005/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:28: virtual ~ControlScope() {}
On 2015/09/30 13:27:36, Michael Starzinger wrote:
> Scope popping seems to be missing from destructor.

Done.

[Michael Starzinger, 2015-10-01 08:55:24]

LGTM from my end.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.h (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:52: ZoneVector<BytecodeLabel>
continue_labels_;
nit: The naming overlap is a little bit confusing I think. Would renaming the
vector to "break_sites" and the label above to "break_target" help? Also
comments could help.

[rmcilroy, 2015-10-01 09:18:56]

A couple of minor comments, but looks great! LGTM once comments are addressed.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:87: } while (current != nullptr);
Do we expect to reach the end of the loop not having executed the command? If
not, UNREACHABLE() after loop.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:198: if (stmt->HasElseStatement()) {
Can we just have this around the else statement - i.e.:
  BytecodeLabel then_end;
  builder()->JumpIfFalse(&then_end);
  Visit(stmt->then_statement());
  
  if (stmt->HasElseStatement()) {
    BytecodeLabel else_end;
    builder()->Jump(&else_end);
    builder()->Bind(&then_end);
    Visit(stmt->else_statement());
    builder()->Bind(&else_end);
  } else {
    builder()->Bind(&then_end);
  }

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:300: builder()->Bind(&condition_start);
nit - put this in the if (stmt->cond())

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:29: class ControlScopeForBreakable;
doesn't exist?

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:14: BindLabels(break_label(),
&break_labels_);
Either move this to an explicit function or call this LoopBuilderScope to make
it clear that it is scoped.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.h (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:40: BytecodeLabel* continue_label() {
return &continue_label_; }
As discussed offline, instead of this could we have a BindContinue operation on
the LoopBuilder and just stash the bind point, then do the binding of the
continue labels in a for loop (which should avoid the need for the the new
Bind(BytecodeLabel* unbound, BytecodeLabel* bound) (instead just needing one
which passes in the explicit offset).

https://codereview.chromium.org/1373903005/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/src/objects.cc#newcode1...
src/objects.cc:12511: bytecode == interpreter::Bytecode::kJumpIfFalse) {
nit - could we make a helper for IsJump, IsJumpConstant.

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:945: {unused, unused, unused,
unused}},
Do you need the constant pool checks? Looks this is always unused.

[oth, 2015-10-01 11:43:03]

Thanks for the feedback. Revised IfBuilder. PTAL. Thanks!

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:87: } while (current != nullptr);
On 2015/10/01 09:18:56, rmcilroy wrote:
> Do we expect to reach the end of the loop not having executed the command? If
> not, UNREACHABLE() after loop.

Done.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:198: if (stmt->HasElseStatement()) {
On 2015/10/01 09:18:55, rmcilroy wrote:
> Can we just have this around the else statement - i.e.:
>   BytecodeLabel then_end;
>   builder()->JumpIfFalse(&then_end);
>   Visit(stmt->then_statement());
>   
>   if (stmt->HasElseStatement()) {
>     BytecodeLabel else_end;
>     builder()->Jump(&else_end);
>     builder()->Bind(&then_end);
>     Visit(stmt->else_statement());
>     builder()->Bind(&else_end);
>   } else {
>     builder()->Bind(&then_end);
>   }
>    

Done.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:300: builder()->Bind(&condition_start);
On 2015/10/01 09:18:55, rmcilroy wrote:
> nit - put this in the if (stmt->cond())

Done.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:29: class ControlScopeForBreakable;
On 2015/10/01 09:18:56, rmcilroy wrote:
> doesn't exist?

Done.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:14: BindLabels(break_label(),
&break_labels_);
On 2015/10/01 09:18:56, rmcilroy wrote:
> Either move this to an explicit function or call this LoopBuilderScope to make
> it clear that it is scoped.

Done.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.h (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:40: BytecodeLabel* continue_label() {
return &continue_label_; }
On 2015/10/01 09:18:56, rmcilroy wrote:
> As discussed offline, instead of this could we have a BindContinue operation
on
> the LoopBuilder and just stash the bind point, then do the binding of the
> continue labels in a for loop (which should avoid the need for the the new
> Bind(BytecodeLabel* unbound, BytecodeLabel* bound) (instead just needing one
> which passes in the explicit offset).

The code is re-worked to partially accommodate this comment and Michi's comment
on naming and clarity.

In new regime, there's no continue_label()/break_label() nor code in the
destructor. Instead, there's methods that called SetContinueTarget and
SetBreakTarget that are called after the loop construction is done. These take a
label set during loop construction and update all the referring sites.
 
Bind(unbound_label, bound_label) is updated such that the bound argument is
pointer to a const to make things clearer.

PTAL.

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:52: ZoneVector<BytecodeLabel>
continue_labels_;
On 2015/10/01 08:55:24, Michael Starzinger wrote:
> nit: The naming overlap is a little bit confusing I think. Would renaming the
> vector to "break_sites" and the label above to "break_target" help? Also
> comments could help.

Done.

https://codereview.chromium.org/1373903005/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/src/objects.cc#newcode1...
src/objects.cc:12511: bytecode == interpreter::Bytecode::kJumpIfFalse) {
On 2015/10/01 09:18:56, rmcilroy wrote:
> nit - could we make a helper for IsJump, IsJumpConstant.

Done.

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:945: {unused, unused, unused,
unused}},
On 2015/10/01 09:18:56, rmcilroy wrote:
> Do you need the constant pool checks? Looks this is always unused.

The only reason unused exists is to explicitly initialize handles. Compiler
quite reasonably complains otherwise.

[Michael Starzinger, 2015-10-01 11:49:37]

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
File src/interpreter/control-flow-builders.h (right):

https://codereview.chromium.org/1373903005/diff/60001/src/interpreter/control...
src/interpreter/control-flow-builders.h:40: BytecodeLabel* continue_label() {
return &continue_label_; }
On 2015/10/01 11:43:02, oth wrote:
> On 2015/10/01 09:18:56, rmcilroy wrote:
> > As discussed offline, instead of this could we have a BindContinue operation
> on
> > the LoopBuilder and just stash the bind point, then do the binding of the
> > continue labels in a for loop (which should avoid the need for the the new
> > Bind(BytecodeLabel* unbound, BytecodeLabel* bound) (instead just needing one
> > which passes in the explicit offset).
> 
> The code is re-worked to partially accommodate this comment and Michi's
comment
> on naming and clarity.
> 
> In new regime, there's no continue_label()/break_label() nor code in the
> destructor. Instead, there's methods that called SetContinueTarget and
> SetBreakTarget that are called after the loop construction is done. These take
a
> label set during loop construction and update all the referring sites.
>  
> Bind(unbound_label, bound_label) is updated such that the bound argument is
> pointer to a const to make things clearer.
> 
> PTAL.

Acknowledged. Works for me, it's clearer now.

[rmcilroy, 2015-10-01 12:12:08]

A last few nits, but LGTM!

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:945: {unused, unused, unused,
unused}},
On 2015/10/01 11:43:02, oth wrote:
> On 2015/10/01 09:18:56, rmcilroy wrote:
> > Do you need the constant pool checks? Looks this is always unused.
> 
> The only reason unused exists is to explicitly initialize handles. Compiler
> quite reasonably complains otherwise.

Right, but if none of the tests have any constants you can just make the
snippets of type ExpectedSnippet<int> and then you don't need to pass anything
for the constant pool fields (e.g.: TEST(PrimitiveExpressions))

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (left):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:350: DCHECK_GE(delta, 0);
Maybe make this DCHECK_NE(delta, 0) instead of removing entirely?

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:330: bool
BytecodeArrayBuilder::IsJumpWithImm8Operand(Bytecode jump_bytecode) {
replace this with your newly added Bytecodes::IsJump please.

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:91: const BytecodeLabel* const bound);
const ref instead? Also, the names still confuse me. How about:
  BytecodeArrayBuilder& Bind(BytecodeLabel* label,
                             const BytecodeLabel& target);

?

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/control...
File src/interpreter/control-flow-builders.cc (right):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:13: DCHECK_EQ(continue_sites_.size(),
0);
nit - DCHECK(continue_sites_.empty());

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:40: labels->resize(0);
nit - labels->clear() ?

[oth, 2015-10-01 13:13:56]

Thanks!

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1373903005/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:945: {unused, unused, unused,
unused}},
On 2015/10/01 12:12:07, rmcilroy wrote:
> On 2015/10/01 11:43:02, oth wrote:
> > On 2015/10/01 09:18:56, rmcilroy wrote:
> > > Do you need the constant pool checks? Looks this is always unused.
> > 
> > The only reason unused exists is to explicitly initialize handles. Compiler
> > quite reasonably complains otherwise.
> 
> Right, but if none of the tests have any constants you can just make the
> snippets of type ExpectedSnippet<int> and then you don't need to pass anything
> for the constant pool fields (e.g.: TEST(PrimitiveExpressions))

Done.

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (left):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:350: DCHECK_GE(delta, 0);
On 2015/10/01 12:12:07, rmcilroy wrote:
> Maybe make this DCHECK_NE(delta, 0) instead of removing entirely?

Done.

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:330: bool
BytecodeArrayBuilder::IsJumpWithImm8Operand(Bytecode jump_bytecode) {
On 2015/10/01 12:12:07, rmcilroy wrote:
> replace this with your newly added Bytecodes::IsJump please.

Done.

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:91: const BytecodeLabel* const bound);
On 2015/10/01 12:12:07, rmcilroy wrote:
> const ref instead? Also, the names still confuse me. How about:
>   BytecodeArrayBuilder& Bind(BytecodeLabel* label,
>                              const BytecodeLabel& target);
> 
> ?

As discussed, changed to:

BytecodeArrayBuilder::Bind(
const BytecodeLabel& target, BytecodeLabel* label) {

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/control...
File src/interpreter/control-flow-builders.cc (right):

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:13: DCHECK_EQ(continue_sites_.size(),
0);
On 2015/10/01 12:12:08, rmcilroy wrote:
> nit - DCHECK(continue_sites_.empty());

Done.

https://codereview.chromium.org/1373903005/diff/80001/src/interpreter/control...
src/interpreter/control-flow-builders.cc:40: labels->resize(0);
On 2015/10/01 12:12:07, rmcilroy wrote:
> nit - labels->clear() ?

Done.

[oth, 2015-10-01 13:52:56]

The CQ bit was checked by oth@chromium.org

[oth, 2015-10-01 13:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1373903005/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-01 13:53:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373903005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373903005/120001

[commit-bot: I haz the power, 2015-10-01 15:04:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-01 15:04:38]

Patchset 7 (id:??) landed as
https://crrev.com/a7e16e513241f6d62f35e763c14aff5ca445195e
Cr-Commit-Position: refs/heads/master@{#31052}