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-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.
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-graph-builder.cc#newcode137
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-graph-builder.cc#newcode144
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-graph-builder.cc#newcode193
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-graph-builder.cc#newcode321
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-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_;
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-graph-builder.h#newcode166
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/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));
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/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;
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/test-run-bytecode-graph-builder.cc#newcode187
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/test-run-bytecode-graph-builder.cc#newcode258
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.
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.
