[Interpreter] Bytecode graph builder

[Interpreter] Skeleton bytecode graph builder

Add skeleton version bytecode-graph-builder.{h,cc} for existing
bytecodes.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/8df7b4f6b502e2c318b61ce604332d51544081c6
Cr-Commit-Position: refs/heads/master@{#30687}

[oth, 2015-08-20 16:18:05]

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

[oth, 2015-08-20 16:18:06]

mstarzinger@chromium.org
rmcilroy@chromium.org
titzer@chromium.org

PTAL. This is just a start. Input on wiring up graphs for the simple bytecodes
we have today would be appreciated. Thanks!

[oth, 2015-09-01 12:25:48]

On 2015/08/20 16:18:06, oth wrote:
> mailto:mstarzinger@chromium.org
> mailto:rmcilroy@chromium.org
> mailto:titzer@chromium.org
> 
> PTAL. This is just a start. Input on wiring up graphs for the simple bytecodes
> we have today would be appreciated. Thanks!

Hi all, this is a little further along. There's not yet a test that drives this
all the way through the compiler, but it's generating graphs.

If you have any comments, they'd be greatly appreciated.

Thanks

[rmcilroy, 2015-09-01 14:00:06]

Looking good to me. I don't have enough knowledge on TF to comment on the graph
building questions, but made a couple of readability nits.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:21: // NB Nodes talk slides:
http://shortn/_fmVf0TjCC4
nit - probably remove the internal talk link

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:46:
values()->push_back(undefined_constant);
nit - comment that this is the accumulator

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:53:
values()->push_back(values()->at(values_index));
Do we need to keep the old register values around (e.g., here where we are
pushing back the existing Node* for the register to the end of the vector before
replacing it with the new node)? Seems like we should be OK just dropping them
on the floor since we won't need them after this point.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:66:
BindRegister(accumulator_pseudo_register(), node);
nit - could we just have a separate Node* accumulator_ in the Environment rather
than a accumulator_pseudo_register which points to a location in values to make
things a bit clearer?

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:117: int locals_count =
bytecode_array()->frame_size() / kPointerSize;
optional nit - you could add a locals_count() helper on BytecodeArray object for
this if you like.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:170: // TODO(oth): write
ast-graph-builder equivalent.
not sure what this TODO means, could you add more detail or remove the TODO if
it's no longer applicable

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:207: Handle<Object> constant =
FixedArray::get(constants, operand_offset);
nit - create a helper for getting a given constant?

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph);
Could we just have the one constructor and one CreateGraph and have the test
framework do the necessary work to create an appropriate CompilationInfo rather
than having test specific entry points?

[oth, 2015-09-01 15:25:58]

Thanks!

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:21: // NB Nodes talk slides:
http://shortn/_fmVf0TjCC4
On 2015/09/01 14:00:06, rmcilroy wrote:
> nit - probably remove the internal talk link

Done.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:46:
values()->push_back(undefined_constant);
On 2015/09/01 14:00:06, rmcilroy wrote:
> nit - comment that this is the accumulator

Moved accumulator out of values per later review comment.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:53:
values()->push_back(values()->at(values_index));
On 2015/09/01 14:00:06, rmcilroy wrote:
> Do we need to keep the old register values around (e.g., here where we are
> pushing back the existing Node* for the register to the end of the vector
before
> replacing it with the new node)? Seems like we should be OK just dropping them
> on the floor since we won't need them after this point.

Done.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:66:
BindRegister(accumulator_pseudo_register(), node);
On 2015/09/01 14:00:06, rmcilroy wrote:
> nit - could we just have a separate Node* accumulator_ in the Environment
rather
> than a accumulator_pseudo_register which points to a location in values to
make
> things a bit clearer?

Done.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:117: int locals_count =
bytecode_array()->frame_size() / kPointerSize;
On 2015/09/01 14:00:06, rmcilroy wrote:
> optional nit - you could add a locals_count() helper on BytecodeArray object
for
> this if you like. 

Done.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:170: // TODO(oth): write
ast-graph-builder equivalent.
On 2015/09/01 14:00:06, rmcilroy wrote:
> not sure what this TODO means, could you add more detail or remove the TODO if
> it's no longer applicable

Done.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:207: Handle<Object> constant =
FixedArray::get(constants, operand_offset);
On 2015/09/01 14:00:06, rmcilroy wrote:
> nit - create a helper for getting a given constant?

Done. Noticed this code is broken anyway - it needs to get the operand at
operand_offset and use that as the index into the constants. Will write test.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph);
On 2015/09/01 14:00:06, rmcilroy wrote:
> Could we just have the one constructor and one CreateGraph and have the test
> framework do the necessary work to create an appropriate CompilationInfo
rather
> than having test specific entry points?

Totally agree with the sentiment. Faking a compilation info looked messy, see
there's a possible example in RawMachineAssemblerTester::Generate() - is this
what you're thinking of.

[Michael Starzinger, 2015-09-02 12:19:22]

Looks sensible from a high-level point of view. No concrete actionable feedback
at this point. I like it. Just left a couple of random ramblings.

https://codereview.chromium.org/1291693004/diff/100001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:151: bool
BytecodeGraphBuilder::CreateGraphForTest(bool stack_check) {
Would be nice to only have one entry point. Especially given the similarity.

https://codereview.chromium.org/1291693004/diff/100001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:27: bool CreateGraph(bool stack_check =
true);
The only reason for the {stack_check} flags is for inlining, where we don't want
sub-graphs to contains a stack. We could just always emit it until we tackle
inlining. Up to you.

https://codereview.chromium.org/1291693004/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:54: Node* NewNode(const Operator* op, bool
incomplete = false) {
The {incomplete} flag is only needed for merge points so that we can
over-reserve space for {Phi}, {Merge} and {Loop} nodes and insert inputs later
without thrashing zone. This will totally depend on how control flow is modeled
in the byte-code. If number of control flow-inputs at every merge point is
known, we won't need it. But I assume we won't know.

https://codereview.chromium.org/1291693004/diff/100001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:75: void
UpdateControlDependencyToLeaveFunction(Node* exit);
Again depends on how control flow is modeled. Only needed if there are "return"
and "throw" byte-codes that represent non-local control flow.

[rmcilroy, 2015-09-02 12:45:24]

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph);
On 2015/09/01 15:25:57, oth wrote:
> On 2015/09/01 14:00:06, rmcilroy wrote:
> > Could we just have the one constructor and one CreateGraph and have the test
> > framework do the necessary work to create an appropriate CompilationInfo
> rather
> > than having test specific entry points?
> 
> Totally agree with the sentiment. Faking a compilation info looked messy, see
> there's a possible example in RawMachineAssemblerTester::Generate() - is this
> what you're thinking of.

Yes, something like RawMachineAssemblerTester::Generate() in a test would be
fine to me. I would rather have slightly messy code in a test than two entry
points if it's possible.

[Michael Starzinger, 2015-09-02 12:46:53]

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph);
On 2015/09/02 12:45:24, rmcilroy wrote:
> On 2015/09/01 15:25:57, oth wrote:
> > On 2015/09/01 14:00:06, rmcilroy wrote:
> > > Could we just have the one constructor and one CreateGraph and have the
test
> > > framework do the necessary work to create an appropriate CompilationInfo
> > rather
> > > than having test specific entry points?
> > 
> > Totally agree with the sentiment. Faking a compilation info looked messy,
see
> > there's a possible example in RawMachineAssemblerTester::Generate() - is
this
> > what you're thinking of.
> 
> Yes, something like RawMachineAssemblerTester::Generate() in a test would be
> fine to me. I would rather have slightly messy code in a test than two entry
> points if it's possible.

+1

[Benedikt Meurer, 2015-09-03 05:02:54]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-09-03 05:02:55]

Graph creation skeleton looks good to me.

[oth, 2015-09-03 11:38:42]

Thanks all. With Ross's help we can run simple graphs from the bytecode graph
builder into turbofan.

I'm going to be working on unit tests for the next day or so, but the main code
is now operational and about ready for review.

Orion

[Michael Starzinger, 2015-09-03 12:19:40]

LGTM. Just nits and a suggestions.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:25: locals_count_(locals_count),
nit: Wouldn't it better fit the semantics if this were called "register_count"
instead?

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:134: // The additional count items are
for the context and closure. as
nit: Drop trailing "as".

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:139: int locals_count =
bytecode_array()->local_count();
nit: Likewise "register_count" here.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:172: for (int i = 0; i <
bytecode_array()->length(); i += last_bytecode_length) {
Would it be more readable to write the loop with less local variable? I was
thinking along the lines of the following. Just a suggestion, pick whichever you
think is more readable ...

int offset = 0
while (offset < bytecode_array()->length()) {
  interpreter::Bytecode bytecode = bytecode_at(offset);
  switch (...) { ... VisitFoo(offset + 1) ... }
  offset += 1 + interpreter::Bytecodes::Size(bytecode);
}

https://codereview.chromium.org/1291693004/diff/140001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1291693004/diff/140001/src/objects.h#newcode4163
src/objects.h:4163: inline int local_count() const;
nit: Wouldn't it better fit the semantics to call this "register_count" instead?

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/cctest.gyp...
test/cctest/cctest.gyp:55: 'compiler/test-run-bytecode-graph-builder.cc',
nit: Please keep list alpha-sorted.

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:37:
isolate->factory()->undefined_value(), sizeof...(args),
Wow, "sizeof...", C++ never seizes to surprise me.

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:64:
i::FLAG_ignition_filter = kFunctionName;
This assignment could potentially trigger double-free situations. Whenever we
start passing --ingnition-filter=foo on the command line to this cctest, the
flags parsing will try to free the static const. I am fine with leaving it as it
is, just be warned that it will hit us eventually.

[rmcilroy, 2015-09-03 12:41:58]

Looking really good, mostly just nits with one optional suggestion.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:38: // array.
nit - update comment (values are no longer pushed to the back of the array)

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver
I think the receiver is just Parameter(-1), so you could probably push that as: 

 const Operator* op = common()->Parameter(-1, nullptr);
 Node* parameter = builder->graph()->NewNode(op, builder->graph()->start());
 values()->push_back(parameter);

if you want it here.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:63: return
the_register.ToParameterIndex(parameters_count());
Does this work - I think the receiver will come back as ParameterIndex '0' and
arg0 will come back as ParameterIndex '1' (so I think you need to push the
receiver into values first before the other parameters).

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:164: // TODO(oth): review
ast-graph-builder equivalent, ie arguments
nit - /s/ie/i.e.,

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:39: // Convert values in bytecode_array to
convenient to handle forms.
nit - I would just replace this with a comment describing bytecode_at

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:41: // Get constant from operand at
position |offset| in bytecode array.
nit - could you make the comment clearer that this get's the constant in the
constant pool which is specified by the index operand at position |offset| in
the bytecode array.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:46: interpreter::Register register_at(int
offset) const;
Optional suggestion (fine with doing this in a later CL if you agree it would be
worth while). Could we encapsulate these operations in something like a
BytecodeArrayWrapper (or some better name) class - i.e.:

class BytecodeArrayWrapper {
 public:
  BytecodeArrayWrapper(BytecodeArrayWrapper);
  
  void next();

  interpreter::Bytecode current_bytecode();
  interpreter::Register register_at(int operand_index);
  Handle<Object> constant_at(int operand_index);
  int8_t smi8_at(int operand_index);

 private:
  size_t offset;
}

That way, next() could advance by Bytecodes::Size(current_bytecode()) and the
register_at, constant_at, smi8_at etc. could all DCHECK that the operands are
the expected type - i.e.:
  DCHECK_EQ(interpreter::OperandType::kImm8,
            interpreter::Bytecodes::GetOperandType(current_bytecode(),
operand_index));

And then the VisitBytecodeX(int offset) functions could be replaced with
VisitBytecodeX(const ByteCodeArrayWrapper& bytecode);

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:76: void FinishBinaryOperation(Node*
node);
How about we replace this with "Node* BuildBinaryOp(Node* left, Node* right,
Token::Value op);" like ASTGraphBuilder?". I.e., instead of:
  Node* node = NewNode(js_op, left, right);
  FinishBinaryOperation(node);

just:
  Node* node = BuildBinaryOp(js_op, left, right);

[Michael Starzinger, 2015-09-03 12:52:07]

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver
On 2015/09/03 12:41:57, rmcilroy wrote:
> I think the receiver is just Parameter(-1), so you could probably push that
as: 
> 
>  const Operator* op = common()->Parameter(-1, nullptr);
>  Node* parameter = builder->graph()->NewNode(op, builder->graph()->start());
>  values()->push_back(parameter);
> 
> if you want it here.

Please use Linkage::kJSFunctionCallClosureParamIndex in that case!

[Michael Starzinger, 2015-09-03 12:55:19]

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver
On 2015/09/03 12:52:07, Michael Starzinger wrote:
> On 2015/09/03 12:41:57, rmcilroy wrote:
> > I think the receiver is just Parameter(-1), so you could probably push that
> as: 
> > 
> >  const Operator* op = common()->Parameter(-1, nullptr);
> >  Node* parameter = builder->graph()->NewNode(op, builder->graph()->start());
> >  values()->push_back(parameter);
> > 
> > if you want it here.
> 
> Please use Linkage::kJSFunctionCallClosureParamIndex in that case!

Ah, sorry, this is about reveiver, not callee ... hmm ... can we in that case
add kInterpreterReceiverParamIndex to Linkage? Magic -1 is scarry. :/

[oth, 2015-09-04 11:06:36]

Thanks for the comments, definitely moving in the right direction. Unit tests
for the bytecode graph builder are next up.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:25: locals_count_(locals_count),
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> nit: Wouldn't it better fit the semantics if this were called "register_count"
> instead?

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:38: // array.
On 2015/09/03 12:41:57, rmcilroy wrote:
> nit - update comment (values are no longer pushed to the back of the array)

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver
On 2015/09/03 12:55:19, Michael Starzinger wrote:
> On 2015/09/03 12:52:07, Michael Starzinger wrote:
> > On 2015/09/03 12:41:57, rmcilroy wrote:
> > > I think the receiver is just Parameter(-1), so you could probably push
that
> > as: 
> > > 
> > >  const Operator* op = common()->Parameter(-1, nullptr);
> > >  Node* parameter = builder->graph()->NewNode(op,
builder->graph()->start());
> > >  values()->push_back(parameter);
> > > 
> > > if you want it here.
> > 
> > Please use Linkage::kJSFunctionCallClosureParamIndex in that case!
> 
> Ah, sorry, this is about reveiver, not callee ... hmm ... can we in that case
> add kInterpreterReceiverParamIndex to Linkage? Magic -1 is scarry. :/

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:63: return
the_register.ToParameterIndex(parameters_count());
On 2015/09/03 12:41:57, rmcilroy wrote:
> Does this work - I think the receiver will come back as ParameterIndex '0' and
> arg0 will come back as ParameterIndex '1' (so I think you need to push the
> receiver into values first before the other parameters).

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:134: // The additional count items are
for the context and closure. as
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> nit: Drop trailing "as".

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:139: int locals_count =
bytecode_array()->local_count();
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> nit: Likewise "register_count" here.

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:164: // TODO(oth): review
ast-graph-builder equivalent, ie arguments
On 2015/09/03 12:41:57, rmcilroy wrote:
> nit - /s/ie/i.e.,

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:172: for (int i = 0; i <
bytecode_array()->length(); i += last_bytecode_length) {
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> Would it be more readable to write the loop with less local variable? I was
> thinking along the lines of the following. Just a suggestion, pick whichever
you
> think is more readable ...
> 
> int offset = 0
> while (offset < bytecode_array()->length()) {
>   interpreter::Bytecode bytecode = bytecode_at(offset);
>   switch (...) { ... VisitFoo(offset + 1) ... }
>   offset += 1 + interpreter::Bytecodes::Size(bytecode);
> }

Yes, good point. Have gone with the suggestion for the BytecodeArrayIterator as
that's potentially useful elsewhere (disassembler).

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:39: // Convert values in bytecode_array to
convenient to handle forms.
On 2015/09/03 12:41:57, rmcilroy wrote:
> nit - I would just replace this with a comment describing bytecode_at

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:41: // Get constant from operand at
position |offset| in bytecode array.
On 2015/09/03 12:41:58, rmcilroy wrote:
> nit - could you make the comment clearer that this get's the constant in the
> constant pool which is specified by the index operand at position |offset| in
> the bytecode array.

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:46: interpreter::Register register_at(int
offset) const;
On 2015/09/03 12:41:58, rmcilroy wrote:
> Optional suggestion (fine with doing this in a later CL if you agree it would
be
> worth while). Could we encapsulate these operations in something like a
> BytecodeArrayWrapper (or some better name) class - i.e.:
> 
> class BytecodeArrayWrapper {
>  public:
>   BytecodeArrayWrapper(BytecodeArrayWrapper);
>   
>   void next();
> 
>   interpreter::Bytecode current_bytecode();
>   interpreter::Register register_at(int operand_index);
>   Handle<Object> constant_at(int operand_index);
>   int8_t smi8_at(int operand_index);
> 
>  private:
>   size_t offset;
> }
> 
> That way, next() could advance by Bytecodes::Size(current_bytecode()) and the
> register_at, constant_at, smi8_at etc. could all DCHECK that the operands are
> the expected type - i.e.:
>   DCHECK_EQ(interpreter::OperandType::kImm8,
>             interpreter::Bytecodes::GetOperandType(current_bytecode(),
> operand_index));
> 
> And then the VisitBytecodeX(int offset) functions could be replaced with
> VisitBytecodeX(const ByteCodeArrayWrapper& bytecode);

Done with BytecodeArrayIterator.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:76: void FinishBinaryOperation(Node*
node);
On 2015/09/03 12:41:57, rmcilroy wrote:
> How about we replace this with "Node* BuildBinaryOp(Node* left, Node* right,
> Token::Value op);" like ASTGraphBuilder?". I.e., instead of:
>   Node* node = NewNode(js_op, left, right);
>   FinishBinaryOperation(node);
> 
> just:
>   Node* node = BuildBinaryOp(js_op, left, right);

Done.

https://codereview.chromium.org/1291693004/diff/140001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1291693004/diff/140001/src/objects.h#newcode4163
src/objects.h:4163: inline int local_count() const;
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> nit: Wouldn't it better fit the semantics to call this "register_count"
instead?

Done.

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/cctest.gyp...
test/cctest/cctest.gyp:55: 'compiler/test-run-bytecode-graph-builder.cc',
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> nit: Please keep list alpha-sorted.

Done.

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:64:
i::FLAG_ignition_filter = kFunctionName;
On 2015/09/03 12:19:40, Michael Starzinger wrote:
> This assignment could potentially trigger double-free situations. Whenever we
> start passing --ingnition-filter=foo on the command line to this cctest, the
> flags parsing will try to free the static const. I am fine with leaving it as
it
> is, just be warned that it will hit us eventually.

Thanks, we had another instance of this in the interpreter. Done.

[rmcilroy, 2015-09-04 11:35:37]

I like the iterator :). A few more comments, I'll look in more depth when the
tests are there.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:35: //
nit - drop extra "//" here and at end of comment

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:36: // values_ layout
nit - /s/values_ layout/The layout of values_ is:/

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:45: // receiver
nit - /s/receiver/Reciever./ (and similar below)

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:71: // values layout is [receiver]
[parameters] [registers]
nit - remove this comment (it will probably end up getting out of sync with the
same comment in Environment()).

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:285: BuildBinaryOp(node);
I was meaning that the BuildBinaryOp would take js_op, left, right and build the
node, rather than just finishing the node once it's done (i.e., move the Node*
node = NewNode(js_op, left, right) in each VisitAdd/Sub... into the
BuildBinaryOp.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h...
src/compiler/linkage.h:333: static const int kInterpreterReceiverParameter = -1;
No, please don't make this an Interpreter parameter - these are only the
parameters which are passed via the Dispatch TailCalls in the bytecode handler.
It should probably be just below kJSFunctionCallClosureParamIndex and be called
something like kJSFunctionRecieverParamIndex (since this is not specific to the
interpreter).

Actually, I'm wondering why this isn't 
Linkage::kJSFunctionCallClosureParamIndex - this seems to be what
ASTGraphBuilder is using for the same thing. Michi, why didn't you want this to
be Linkage::kJSFunctionCallClosureParamIndex?

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:15: :
bytecode_array_(bytecode_array), bytecode_offset_(0) {
need to init operands_used_ if DEBUG

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:31: bool
BytecodeArrayIterator::More() const {
More seems a little confusing here, since it implies you could run until More()
and still call current_bytecode() even if More != true (since it implies that
you can't call Next, not that we have gone past the current value). How about
done() (like StackFrameIteratorBase)? (also maybe Advance() instead of Next()
also like StackFrameIteratorBase?)

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:86: void
BytecodeArrayIterator::CheckOperandsUsed() const {
I'm wondering how useful this will be (and whether we will end up needing to
iterate over bytecode and not always check all the bytecode operands). I'm fine
with having it here though if you think it would be useful.

[oth, 2015-09-04 16:15:46]

Thanks, incorporated.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:35: //
On 2015/09/04 11:35:37, rmcilroy wrote:
> nit - drop extra "//" here and at end of comment

Done.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:36: // values_ layout
On 2015/09/04 11:35:37, rmcilroy wrote:
> nit - /s/values_ layout/The layout of values_ is:/

Done.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:45: // receiver
On 2015/09/04 11:35:37, rmcilroy wrote:
> nit - /s/receiver/Reciever./ (and similar below)

Done.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:71: // values layout is [receiver]
[parameters] [registers]
On 2015/09/04 11:35:37, rmcilroy wrote:
> nit - remove this comment (it will probably end up getting out of sync with
the
> same comment in Environment()).

Done.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:285: BuildBinaryOp(node);
On 2015/09/04 11:35:37, rmcilroy wrote:
> I was meaning that the BuildBinaryOp would take js_op, left, right and build
the
> node, rather than just finishing the node once it's done (i.e., move the Node*
> node = NewNode(js_op, left, right) in each VisitAdd/Sub... into the
> BuildBinaryOp.

Done.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h...
src/compiler/linkage.h:333: static const int kInterpreterReceiverParameter = -1;
On 2015/09/04 11:35:37, rmcilroy wrote:
> No, please don't make this an Interpreter parameter - these are only the
> parameters which are passed via the Dispatch TailCalls in the bytecode
handler.
> It should probably be just below kJSFunctionCallClosureParamIndex and be
called
> something like kJSFunctionRecieverParamIndex (since this is not specific to
the
> interpreter).
> 
> Actually, I'm wondering why this isn't 
> Linkage::kJSFunctionCallClosureParamIndex - this seems to be what
> ASTGraphBuilder is using for the same thing. Michi, why didn't you want this
to
> be Linkage::kJSFunctionCallClosureParamIndex?

Removed. Won't commit until this is resolved. Comment added to
BytecodeGraphBuilder next to -1.

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:15: :
bytecode_array_(bytecode_array), bytecode_offset_(0) {
On 2015/09/04 11:35:37, rmcilroy wrote:
> need to init operands_used_ if DEBUG

Yes, but that's what MarkOperandsUnused() does.

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:31: bool
BytecodeArrayIterator::More() const {
On 2015/09/04 11:35:37, rmcilroy wrote:
> More seems a little confusing here, since it implies you could run until
More()
> and still call current_bytecode() even if More != true (since it implies that
> you can't call Next, not that we have gone past the current value). How about
> done() (like StackFrameIteratorBase)? (also maybe Advance() instead of Next()
> also like StackFrameIteratorBase?)

Done. These were adopted from elsewhere in the tree.

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:86: void
BytecodeArrayIterator::CheckOperandsUsed() const {
On 2015/09/04 11:35:37, rmcilroy wrote:
> I'm wondering how useful this will be (and whether we will end up needing to
> iterate over bytecode and not always check all the bytecode operands). I'm
fine
> with having it here though if you think it would be useful.

Gone :-)

[Michael Starzinger, 2015-09-04 16:35:51]

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h...
src/compiler/linkage.h:333: static const int kInterpreterReceiverParameter = -1;
On 2015/09/04 16:15:46, oth wrote:
> On 2015/09/04 11:35:37, rmcilroy wrote:
> > No, please don't make this an Interpreter parameter - these are only the
> > parameters which are passed via the Dispatch TailCalls in the bytecode
> handler.
> > It should probably be just below kJSFunctionCallClosureParamIndex and be
> called
> > something like kJSFunctionRecieverParamIndex (since this is not specific to
> the
> > interpreter).
> > 
> > Actually, I'm wondering why this isn't 
> > Linkage::kJSFunctionCallClosureParamIndex - this seems to be what
> > ASTGraphBuilder is using for the same thing. Michi, why didn't you want this
> to
> > be Linkage::kJSFunctionCallClosureParamIndex?
> 
> Removed. Won't commit until this is resolved. Comment added to
> BytecodeGraphBuilder next to -1.

Receiver and closure are to different things. One refers to whatever "this"
points to. The other refers to the JSFunction that the activation representing.

And I agree this is not a "Parameter" to the handler stub this is a "ParamIndex"
for the function activation, that's again something different.

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:45: // Candidate:-
Linkage::kJSFunctionCallClosureParamIndex
Nope, not a candidate!

[Michael Starzinger, 2015-09-04 16:46:58]

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op =
common()->Parameter(-1, nullptr);
Something is highly fishy here. Parameter(0) should refer to the receiver.
Essentially if you call "x.f(a,b,c)", then ...

Parameter(0) == x          // Receiver
Parameter(1) == a          // 1st arg
Parameter(2) == b          // 2nd arg
Parameter(3) == c          // 3rd arg
Parameter(4) == f.context  // Context
Parameter(-1) == f         // Closure

Anything else would surprise me if it'd work.

[rmcilroy, 2015-09-07 08:38:35]

On 2015/09/04 16:46:58, Michael Starzinger wrote:
>
https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
> src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op =
> common()->Parameter(-1, nullptr);
> Something is highly fishy here. Parameter(0) should refer to the receiver.
> Essentially if you call "x.f(a,b,c)", then ...
> 
> Parameter(0) == x          // Receiver
> Parameter(1) == a          // 1st arg
> Parameter(2) == b          // 2nd arg
> Parameter(3) == c          // 3rd arg
> Parameter(4) == f.context  // Context
> Parameter(-1) == f         // Closure
> 
> Anything else would surprise me if it'd work.

Yeah, I think I was wrong here. I was confused by the comment in ASTGraphBuilder
which says:
  // Bind all parameter variables. The parameter indices are shifted by 1
  // (receiver is parameter index -1 but environment index 0).
From this comment it sounds like the reciever is -1, but I guess what it is
actually saying is that the reciever is '0' and all other parameters are shifted
up by one because of it. Could we fix the comment?

[oth, 2015-09-07 08:40:15]

Thanks Michael, PTAQL.

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op =
common()->Parameter(-1, nullptr);
On 2015/09/04 16:46:58, Michael Starzinger wrote:
> Something is highly fishy here. Parameter(0) should refer to the receiver.
> Essentially if you call "x.f(a,b,c)", then ...
> 
> Parameter(0) == x          // Receiver
> Parameter(1) == a          // 1st arg
> Parameter(2) == b          // 2nd arg
> Parameter(3) == c          // 3rd arg
> Parameter(4) == f.context  // Context
> Parameter(-1) == f         // Closure
> 
> Anything else would surprise me if it'd work.

Right, I think we got confused by the wording of the comment in
ast-graph-builder and lost our way. Hopefully moved back closer this in last
upload.

[Michael Starzinger, 2015-09-07 09:51:31]

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op =
common()->Parameter(-1, nullptr);
On 2015/09/07 08:40:15, oth wrote:
> On 2015/09/04 16:46:58, Michael Starzinger wrote:
> > Something is highly fishy here. Parameter(0) should refer to the receiver.
> > Essentially if you call "x.f(a,b,c)", then ...
> > 
> > Parameter(0) == x          // Receiver
> > Parameter(1) == a          // 1st arg
> > Parameter(2) == b          // 2nd arg
> > Parameter(3) == c          // 3rd arg
> > Parameter(4) == f.context  // Context
> > Parameter(-1) == f         // Closure
> > 
> > Anything else would surprise me if it'd work.
> 
> Right, I think we got confused by the wording of the comment in
> ast-graph-builder and lost our way. Hopefully moved back closer this in last
> upload.

Looks good now.

https://codereview.chromium.org/1291693004/diff/200001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/200001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:65: return parameter_index;
nit: Just "return the_register.ToParameterIndex(parameter_count())" here?

[rmcilroy, 2015-09-10 10:19:43]

Looks really great. All nits except for the comment about TwoParameterTest, but
the rest lgtm!

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:18: // - Need story for context
parameter, closure parameter, this.
nit - I think only closure parameter is relevent here any longer (we have a
story for context parameter and this)

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:137: int register_count =
bytecode_array()->register_count();
nit - you could just inline bytecode_array()->register_count() in the
constructor call below (like you do with parameter_count).

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:144: UNIMPLEMENTED();  // write
ast-graph-builder equivalent.
nit - TODO(oth): Write...

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:193: //  Node* node =
jsgraph()->Int32Constant(iterator.GetSmi8Operand(0));
remove comment

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:321: 
You'll need to declare VisitStoreIC and VisitKeyedStoreIC now too (as
UNIMPLEMENTED()).

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:155: int register_base_;
nit - fields should be at the end (after private methods).

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:166: int
RegisterToValuesIndex(interpreter::Register the_register) const;
nit - put this above the accessors

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:67: return FixedArray::get(constants,
GetIndexOperand(operand_index));
this should be "constants->get(GetIndexOperand(operand_index));" I think.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:65: i::FLAG_ignition =
true;
need to also add i::FLAG_vector_stores now.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:187:
Object::ToString(isolate, param2.second).ToHandleChecked()->ToCString();
This seems overly complicated. You don't actually need to pass the same values
to the code snippet as you pass during the actual call. The values you pass to
the code snippet are just what is going to be used during the "compileRun" to
produce the bytecode, so just passing '(0, 0)' for both would be fine.

I think without this you can get rid of the TwoParameterTest and just use code
snippets with an extra field for parameters - i.e.: add the field
"Handle<Object> parameters[2]" to ExpectedSnippet and do:

size_t num_snippets = sizeof(snippets) / sizeof(snippets[0]);
  for (size_t i = 0; i < num_snippets; i++) {
    ScopedVector<char> script(1024);
    SNPrintF(script, "function %s(p1, p2) { %s }\n%s(0, 0);", kFunctionName,
             snippets[i].code_snippet, kFunctionName);
  
    BytecodeGraphTester tester(handles.main_isolate(), handles.main_zone(),
                               script.start());
    auto callable = tester.GetCallable<Handle<Object>, Handle<Object>>();
    Handle<Object> return_val = callable(snippet[i].param[0],
snippet[i].param[1]).ToHandleChecked();
    CHECK(return_val.is_identical_to(snippets[i].return_val));
  }

Grab me in person if any of this doesn't make sense.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:258: }
nit - could you add a TODO to add a test for constants (e.g, snippets like
"return "test_string";" and "return 0.2" etc.

[oth, 2015-09-10 13:59:06]

Thanks! All comments incorporated. Only big deltas are in
test-run-bytecode-graph-builder.cc.

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:18: // - Need story for context
parameter, closure parameter, this.
On 2015/09/10 10:19:43, rmcilroy wrote:
> nit - I think only closure parameter is relevent here any longer (we have a
> story for context parameter and this)

Done.

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:137: int register_count =
bytecode_array()->register_count();
On 2015/09/10 10:19:43, rmcilroy wrote:
> nit - you could just inline bytecode_array()->register_count() in the
> constructor call below (like you do with parameter_count).

Done.

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:144: UNIMPLEMENTED();  // write
ast-graph-builder equivalent.
On 2015/09/10 10:19:43, rmcilroy wrote:
> nit - TODO(oth): Write...

Done.

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:193: //  Node* node =
jsgraph()->Int32Constant(iterator.GetSmi8Operand(0));
On 2015/09/10 10:19:43, rmcilroy wrote:
> remove comment

Done. Doh!

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:321: 
On 2015/09/10 10:19:43, rmcilroy wrote:
> You'll need to declare VisitStoreIC and VisitKeyedStoreIC now too (as
> UNIMPLEMENTED()).

Acknowledged.

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:155: int register_base_;
On 2015/09/10 10:19:43, rmcilroy wrote:
> nit - fields should be at the end (after private methods).

Done.

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.h:166: int
RegisterToValuesIndex(interpreter::Register the_register) const;
On 2015/09/10 10:19:43, rmcilroy wrote:
> nit - put this above the accessors

Done.

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:67: return FixedArray::get(constants,
GetIndexOperand(operand_index));
On 2015/09/10 10:19:43, rmcilroy wrote:
> this should be "constants->get(GetIndexOperand(operand_index));" I think.

The static version returns a handle for the element that this method returns.
The member version returns bare object pointer. The current path might be
cleaner than cons-ing up handle here. WDYT?

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:65: i::FLAG_ignition =
true;
On 2015/09/10 10:19:43, rmcilroy wrote:
> need to also add i::FLAG_vector_stores now.

Done.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:187:
Object::ToString(isolate, param2.second).ToHandleChecked()->ToCString();
On 2015/09/10 10:19:43, rmcilroy wrote:
> This seems overly complicated. You don't actually need to pass the same values
> to the code snippet as you pass during the actual call. The values you pass to
> the code snippet are just what is going to be used during the "compileRun" to
> produce the bytecode, so just passing '(0, 0)' for both would be fine.
> 
> I think without this you can get rid of the TwoParameterTest and just use code
> snippets with an extra field for parameters - i.e.: add the field
> "Handle<Object> parameters[2]" to ExpectedSnippet and do:
> 
> size_t num_snippets = sizeof(snippets) / sizeof(snippets[0]);
>   for (size_t i = 0; i < num_snippets; i++) {
>     ScopedVector<char> script(1024);
>     SNPrintF(script, "function %s(p1, p2) { %s }\n%s(0, 0);", kFunctionName,
>              snippets[i].code_snippet, kFunctionName);
>   
>     BytecodeGraphTester tester(handles.main_isolate(), handles.main_zone(),
>                                script.start());
>     auto callable = tester.GetCallable<Handle<Object>, Handle<Object>>();
>     Handle<Object> return_val = callable(snippet[i].param[0],
> snippet[i].param[1]).ToHandleChecked();
>     CHECK(return_val.is_identical_to(snippets[i].return_val));
>   }
> 
> Grab me in person if any of this doesn't make sense.

Agree, the missing knowledge was the existence of Object::SameValue() that
avoids the extra work matching object types.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/t...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:258: }
On 2015/09/10 10:19:43, rmcilroy wrote:
> nit - could you add a TODO to add a test for constants (e.g, snippets like
> "return "test_string";" and "return 0.2" etc.

Added double and string snippets to BytecodeGraphBuilderReturnStatements test
and TODO for NaN.

[rmcilroy, 2015-09-10 14:06:28]

Looks great, let's land it!

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:67: return FixedArray::get(constants,
GetIndexOperand(operand_index));
On 2015/09/10 13:59:06, oth wrote:
> On 2015/09/10 10:19:43, rmcilroy wrote:
> > this should be "constants->get(GetIndexOperand(operand_index));" I think.
> 
> The static version returns a handle for the element that this method returns.
> The member version returns bare object pointer. The current path might be
> cleaner than cons-ing up handle here. WDYT?

Ahh you're right sorry. Strange that this is a static method though... but not
your problem :).

[rmcilroy, 2015-09-10 14:06:28]

Looks great, let's land it!

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/byteco...
src/interpreter/bytecode-array-iterator.cc:67: return FixedArray::get(constants,
GetIndexOperand(operand_index));
On 2015/09/10 13:59:06, oth wrote:
> On 2015/09/10 10:19:43, rmcilroy wrote:
> > this should be "constants->get(GetIndexOperand(operand_index));" I think.
> 
> The static version returns a handle for the element that this method returns.
> The member version returns bare object pointer. The current path might be
> cleaner than cons-ing up handle here. WDYT?

Ahh you're right sorry. Strange that this is a static method though... but not
your problem :).

[oth, 2015-09-10 14:19:32]

The CQ bit was checked by oth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-10 14:19:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291693004/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291693004/300001

[commit-bot: I haz the power, 2015-09-10 14:31:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-10 14:31:41]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[oth, 2015-09-10 15:48:54]

The CQ bit was checked by oth@chromium.org

[oth, 2015-09-10 15:48:55]

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/1291693004/#ps340001
(title: "More fixes for bots (tests w/ ASAN and tests w/ slow asserts
enabled).")

[commit-bot: I haz the power, 2015-09-10 15:49:00]

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

[commit-bot: I haz the power, 2015-09-10 16:21:39]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-09-10 16:21:55]

Patchset 18 (id:??) landed as
https://crrev.com/8df7b4f6b502e2c318b61ce604332d51544081c6
Cr-Commit-Position: refs/heads/master@{#30687}

[brucedawson, 2015-09-14 17:43:04]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-09-14 17:43:05]

This mornings 'new warning' report from the /analyzer builder pointed out some
variable shadowing. Not bugs, but worth mentioning.

https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:392: const Operator* op =
common()->IfSuccess();
This shadows the 'op' function parameter, which makes the code slightly more
confusing. Consider renaming to avoid shadowing?

https://codereview.chromium.org/1291693004/diff/340001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:418: Node* inputs[] = {control, other};
This shadows the local 'inputs' variable declared at the top. I'm not sure
whether the types being different makes it more or less confusing.

Consider renaming to avoid shadowing?