I like the iterator :). A few more comments, I'll look in more depth when
the
tests are there.
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode35
src/compiler/bytecode-graph-builder.cc:35: //
nit - drop extra "//" here and at end of comment
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode36
src/compiler/bytecode-graph-builder.cc:36: // values_ layout
nit - /s/values_ layout/The layout of values_ is:/
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode45
src/compiler/bytecode-graph-builder.cc:45: // receiver
nit - /s/receiver/Reciever./ (and similar below)
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode71
src/compiler/bytecode-graph-builder.cc:71: // values layout is
[receiver] [parameters] [registers]
nit - remove this comment (it will probably end up getting out of sync
with the same comment in Environment()).
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode285
src/compiler/bytecode-graph-builder.cc:285: BuildBinaryOp(node);
I was meaning that the BuildBinaryOp would take js_op, left, right and
build the node, rather than just finishing the node once it's done
(i.e., move the Node* node = NewNode(js_op, left, right) in each
VisitAdd/Sub... into the BuildBinaryOp.
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h
File src/compiler/linkage.h (right):
https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h#newcode333
src/compiler/linkage.h:333: static const int
kInterpreterReceiverParameter = -1;
No, please don't make this an Interpreter parameter - these are only the
parameters which are passed via the Dispatch TailCalls in the bytecode
handler. It should probably be just below
kJSFunctionCallClosureParamIndex and be called something like
kJSFunctionRecieverParamIndex (since this is not specific to the
interpreter).
Actually, I'm wondering why this isn't
Linkage::kJSFunctionCallClosureParamIndex - this seems to be what
ASTGraphBuilder is using for the same thing. Michi, why didn't you want
this to be Linkage::kJSFunctionCallClosureParamIndex?
https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc
File src/interpreter/bytecode-array-iterator.cc (right):
https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc#newcode15
src/interpreter/bytecode-array-iterator.cc:15: :
bytecode_array_(bytecode_array), bytecode_offset_(0) {
need to init operands_used_ if DEBUG
https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc#newcode31
src/interpreter/bytecode-array-iterator.cc:31: bool
BytecodeArrayIterator::More() const {
More seems a little confusing here, since it implies you could run until
More() and still call current_bytecode() even if More != true (since it
implies that you can't call Next, not that we have gone past the current
value). How about done() (like StackFrameIteratorBase)? (also maybe
Advance() instead of Next() also like StackFrameIteratorBase?)
https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc#newcode86
src/interpreter/bytecode-array-iterator.cc:86: void
BytecodeArrayIterator::CheckOperandsUsed() const {
I'm wondering how useful this will be (and whether we will end up
needing to iterate over bytecode and not always check all the bytecode
operands). I'm fine with having it here though if you think it would be
useful.
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.
