Looking really good, mostly just nits with one optional suggestion.
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#newcode38
src/compiler/bytecode-graph-builder.cc:38: // array.
nit - update comment (values are no longer pushed to the back of the
array)
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode48
src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver
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.
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());
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).
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
nit - /s/ie/i.e.,
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.
nit - I would just replace this with a comment describing bytecode_at
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.
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.
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;
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);
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);
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);
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.
