On 2015/07/24 18:34:48, rmcilroy wrote:
Not read through the whole thing yet, but a couple of comments /
suggestions
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode5
src/interpreter/bytecodes.cc:5: #define OPERANDS(x) OperandInfo##x
Is this required?
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode17
src/interpreter/bytecodes.cc:17: static const int is_explicit;
instead of having is_explicit, could we just special-case None (and if we
remove
Imm0 and keep all the other operands to be byte size currently this would
simplify things a lot I think - potentially getting rid of the need for
the
OperatorInfo struct). WDYT?
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode29
src/interpreter/bytecodes.cc:29: OPERAND_INFO(Imm0, 0);
What is Imm0? Seems unrequired.
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode31
src/interpreter/bytecodes.cc:31: OPERAND_INFO(Smi, 4);
As mentioned offline, this can't be a 4 byte operand - we should make it
take
an
index into the constant pool (or we can remove it for now and just have
Imm8
for
now).
I actually think we should have all operands be just 1 byte for now for
simplicity until we need something with 16bits or larger range - WDYT?
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode51
src/interpreter/bytecodes.cc:51: PACK_OPERAND(op0, 0) | PACK_OPERAND(op1,
1) |
PACK_OPERAND(op2, 2)
I personally don't think it's worth packing operands into a single
integer -
we
are going to end up with a bytecode which requires more than 4 operands at
some
point and then this will no longer work.
It's difficult to work out how you are using this without the generator
CL,
but
would it work if we had these as a std::vector in BytecodeInfo, and
create a
constant array of BytecodeInfo which can be accessed by index (to avoid
having
to allocate BytecodeInfo's all over the place)?
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode55
src/interpreter/bytecodes.cc:55: #define BYTECODE_INFO(code, op0, op1,
op2)
\
As much as I like pre-processor magic </sarcasm> - does this need to be
done
as
a macro? Couldn't it just be a class or struct which takes the bytecodes
and
operands as constructor arguments, or passing them as template arguments?
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/bytecodes.cc#newcode76
src/interpreter/bytecodes.cc:76: BYTECODE_INFO(kReturn, Operand::kNone,
Operand::kNone, Operand::kNone);
I would really like this list of declarations to be in the same place as
BYTECODE_LIST if possible. Is there any way we could get closer to what
you
had
when you showed me it the other day (i.e., "V(Move, Operands(Reg,
Reg,Reg))"
)?
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):
https://codereview.chromium.org/1257543003/diff/1/src/interpreter/interpreter.cc#newcode58
src/interpreter/interpreter.cc:58: UNIMPLEMENTED();
This will fail when we build the interpreter-table (which we won't want
otherwise we can't test anything). We should figure out a way of
generating
code
which causes an abort when run (not sure how we would do that in TF, but
I'll
have a look).
Thanks! Patch set 2 is a simpler/cleaner implementation based on comments.
https://codereview.chromium.org/1257543003/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
