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).
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.
