[Interpreter] Add more bytecode definitions and add operand types.

src/interpreter/bytecodes.cc

Index: src/interpreter/bytecodes.cc
diff --git a/src/interpreter/bytecodes.cc b/src/interpreter/bytecodes.cc
index 89fcaf60e8e2339eb716ea9db5a900e134b94752..b86bb82094249467bfd7a3692d42f52852e2cc83 100644
--- a/src/interpreter/bytecodes.cc
+++ b/src/interpreter/bytecodes.cc
@@ -2,18 +2,86 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#define OPERANDS(x) OperandInfo##x

[rmcilroy, 2015/07/24 18:34:48]

Is this required?

+
 #include "src/interpreter/bytecodes.h"
 
 namespace v8 {
 namespace internal {
 namespace interpreter {
 
+static const int kMaxOperands = 3;
+
+template <enum Operand O>
+struct OperandInfo {
+  static const int is_explicit;

[rmcilroy, 2015/07/24 18:34:48]

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? 

+  static const int size;
+};
+
+#define OPERAND_INFO(op, sz)                          \
+  template <>                                         \
+  struct OperandInfo<Operand::k##op> {                \
+    static const int is_explicit = (sz == 0) ? 0 : 1; \
+    static const int size = sz;                       \
+  }
+
+OPERAND_INFO(None, 0);
+OPERAND_INFO(Imm0, 0);

[rmcilroy, 2015/07/24 18:34:48]

What is Imm0? Seems unrequired.

+OPERAND_INFO(Imm8, 1);
+OPERAND_INFO(Smi, 4);

[rmcilroy, 2015/07/24 18:34:47]

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? 

+OPERAND_INFO(Reg, 1);
+
+
+template <enum Bytecode B>
+struct BytecodeInfo {
+  // Size of byte code and operands
+  static const int size;
+  // Number of operands
+  static const int number_of_operands;
+  // Packed operand values
+  static const int operands;
+};
+
+
+// Packing operands into a single integer (makes indexing easier)
+static const int kOperandShift = 4;
+STATIC_ASSERT(static_cast<int>(Operand::kLast) < ((1 << kOperandShift) - 1));
+#define PACK_OPERAND(op, i) (static_cast<int>(op) << (i * kOperandShift))
+#define PACK_OPERANDS(op0, op1, op2) \
+  PACK_OPERAND(op0, 0) | PACK_OPERAND(op1, 1) | PACK_OPERAND(op2, 2)

[rmcilroy, 2015/07/24 18:34:48]

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

+#define UNPACK_OPERAND(op, pos) (Operand)(op >> (pos * kOperandShift))
+
+// Generator of BytecodeInfos for each bytecode.
+#define BYTECODE_INFO(code, op0, op1, op2)                                   \

[rmcilroy, 2015/07/24 18:34:48]

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?

+  template <>                                                                \
+  struct BytecodeInfo<Bytecode::code> {                                      \
+    static const int size = 1 + OperandInfo<op0>::size +                     \
+                            OperandInfo<op1>::size + OperandInfo<op2>::size; \
+    static const int number_of_operands = OperandInfo<op0>::is_explicit +    \
+                                          OperandInfo<op1>::is_explicit +    \
+                                          OperandInfo<op2>::is_explicit;     \
+    static const int operands = PACK_OPERANDS(op0, op1, op2);                \
+  }
+
+// Bytecode declarations
+BYTECODE_INFO(kLoadSmi0, Operand::kReg, Operand::kNone, Operand::kNone);
+BYTECODE_INFO(kLoadSmi8, Operand::kReg, Operand::kImm8, Operand::kNone);
+BYTECODE_INFO(kLoadSmi, Operand::kReg, Operand::kSmi, Operand::kNone);
+BYTECODE_INFO(kMove, Operand::kReg, Operand::kReg, Operand::kNone);
+BYTECODE_INFO(kAdd, Operand::kReg, Operand::kReg, Operand::kReg);
+BYTECODE_INFO(kSub, Operand::kReg, Operand::kReg, Operand::kReg);
+BYTECODE_INFO(kMul, Operand::kReg, Operand::kReg, Operand::kReg);
+BYTECODE_INFO(kDiv, Operand::kReg, Operand::kReg, Operand::kReg);
+BYTECODE_INFO(kMod, Operand::kReg, Operand::kReg, Operand::kReg);
+BYTECODE_INFO(kReturn, Operand::kNone, Operand::kNone, Operand::kNone);

[rmcilroy, 2015/07/24 18:34:48]

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))" )?

+
+
 // static
 const char* Bytecodes::ToString(Bytecode bytecode) {
   switch (bytecode) {
-#define CASE(Name, _)       \
-    case Bytecode::k##Name: \
-      return #Name;
+#define CASE(Name)        \
+  case Bytecode::k##Name: \
+    return #Name;
     BYTECODE_LIST(CASE)
 #undef CASE
   }
@@ -23,11 +91,19 @@ const char* Bytecodes::ToString(Bytecode bytecode) {
 
 
 // static
-const int Bytecodes::NumberOfArguments(Bytecode bytecode) {
+Bytecode Bytecodes::FromByte(uint8_t value) {
+  Bytecode code = static_cast<Bytecode>(value);
+  CHECK(code <= Bytecode::kLast);
+  return code;
+}
+
+
+// static
+const int Bytecodes::NumberOfOperands(Bytecode bytecode) {
   switch (bytecode) {
-#define CASE(Name, arg_count)   \
-    case Bytecode::k##Name:     \
-      return arg_count;
+#define CASE(Name)        \
+  case Bytecode::k##Name: \
+    return BytecodeInfo<Bytecode::k##Name>::number_of_operands;
     BYTECODE_LIST(CASE)
 #undef CASE
   }
@@ -37,15 +113,62 @@ const int Bytecodes::NumberOfArguments(Bytecode bytecode) {
 
 
 // static
+const Operand Bytecodes::GetOperand(Bytecode bytecode, int i) {
+  CHECK(i < kMaxOperands);
+
+  int packed = 0;
+  switch (bytecode) {
+#define CASE(Name)        \
+  case Bytecode::k##Name: \
+    packed = BytecodeInfo<Bytecode::k##Name>::operands;
+    BYTECODE_LIST(CASE)
+#undef CASE
+  }
+  return UNPACK_OPERAND(packed, i);
+}
+
+
+// static
+const int Bytecodes::GetOperandSize(Operand operand) {
+  switch (operand) {
+#define CASE(Name)       \
+  case Operand::k##Name: \
+    return OperandInfo<Operand::k##Name>::size;
+    OPERAND_LIST(CASE)
+#undef CASE
+  }
+  UNREACHABLE();
+  return 0;
+}
+
+
+// static
 const int Bytecodes::Size(Bytecode bytecode) {
-  return NumberOfArguments(bytecode) + 1;
+  switch (bytecode) {
+#define CASE(Name)        \
+  case Bytecode::k##Name: \
+    return BytecodeInfo<Bytecode::k##Name>::size;
+    BYTECODE_LIST(CASE)
+#undef CASE
+  }
+  UNREACHABLE();
+  return 0;
 }
 
 
-#define CHECK_SIZE(Name, arg_count) \
-  STATIC_ASSERT(arg_count <= Bytecodes::kMaximumNumberOfArguments);
-  BYTECODE_LIST(CHECK_SIZE)
-#undef CHECK_SIZE
+// static
+const int Bytecodes::MaximumNumberOfOperands() {
+  // TODO(oth): STATIC_ASSERT on number of operands.
+  return kMaxOperands;
+}
+
+
+// static
+const int Bytecodes::MaximumSize() {
+  // TODO(oth): Maximum size of bytecode and associated operands
+  STATIC_ASSERT(BytecodeInfo<Bytecode::kLoadSmi>::size == 6);
+  return 6;
+}
 
 
 std::ostream& operator<<(std::ostream& os, const Bytecode& bytecode) {