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-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode18
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-graph-builder.cc#newcode137
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-graph-builder.cc#newcode144
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-graph-builder.cc#newcode193
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-graph-builder.cc#newcode321
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-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):
https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.h#newcode155
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-graph-builder.h#newcode166
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/bytecode-array-iterator.cc
File src/interpreter/bytecode-array-iterator.cc (right):
https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67
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/test-run-bytecode-graph-builder.cc
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode65
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/test-run-bytecode-graph-builder.cc#newcode187
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/test-run-bytecode-graph-builder.cc#newcode258
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.
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.
