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-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode25
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-graph-builder.cc#newcode38
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-graph-builder.cc#newcode48
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-graph-builder.cc#newcode63
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-graph-builder.cc#newcode134
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-graph-builder.cc#newcode139
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-graph-builder.cc#newcode164
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-graph-builder.cc#newcode172
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-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.h#newcode39
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-graph-builder.h#newcode41
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-graph-builder.h#newcode46
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-graph-builder.h#newcode76
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#newcode55
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/test-run-bytecode-graph-builder.cc
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode64
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.
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.
