Thanks!
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
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-graph-builder.cc#newcode46
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-graph-builder.cc#newcode53
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-graph-builder.cc#newcode66
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-graph-builder.cc#newcode117
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-graph-builder.cc#newcode170
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-graph-builder.cc#newcode207
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-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);
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.
https://codereview.chromium.org/1291693004/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
