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-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode21
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-graph-builder.cc#newcode46
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-graph-builder.cc#newcode53
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-graph-builder.cc#newcode66
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-graph-builder.cc#newcode117
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-graph-builder.cc#newcode170
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-graph-builder.cc#newcode207
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-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):
https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22
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?
https://codereview.chromium.org/1291693004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
