[interpreter] Streamline bytecode array writing.

src/interpreter/bytecode-array-writer.cc

Index: src/interpreter/bytecode-array-writer.cc
diff --git a/src/interpreter/bytecode-array-writer.cc b/src/interpreter/bytecode-array-writer.cc
index c4760429e491688f6f2992c585aaad5a6e7024bc..1dc39af144c00802db10166b2606de2283292059 100644
--- a/src/interpreter/bytecode-array-writer.cc
+++ b/src/interpreter/bytecode-array-writer.cc
@@ -13,6 +13,9 @@ namespace v8 {
 namespace internal {
 namespace interpreter {
 
+STATIC_CONST_MEMBER_DEFINITION const size_t
+    BytecodeArrayWriter::kMaxSizeOfPackedBytecode;
+
 BytecodeArrayWriter::BytecodeArrayWriter(
     Isolate* isolate, Zone* zone, ConstantArrayBuilder* constant_array_builder)
     : isolate_(isolate),
@@ -132,7 +135,8 @@ OperandScale GetOperandScale(const BytecodeNode* const node) {
   const OperandTypeInfo* operand_type_infos =
       Bytecodes::GetOperandTypeInfos(node->bytecode());
   OperandScale operand_scale = OperandScale::kSingle;
-  for (int i = 0; i < node->operand_count(); ++i) {
+  int operand_count = node->operand_count();
+  for (int i = 0; i < operand_count; ++i) {
     switch (operand_type_infos[i]) {
       case OperandTypeInfo::kScalableSignedByte: {
         uint32_t operand = node->operand(i);
@@ -162,55 +166,58 @@ OperandScale GetOperandScale(const BytecodeNode* const node) {
 void BytecodeArrayWriter::EmitBytecode(const BytecodeNode* const node) {
   DCHECK_NE(node->bytecode(), Bytecode::kIllegal);
 
+  uint8_t buffer[kMaxSizeOfPackedBytecode];
+  uint8_t* buffer_limit = buffer;
+
   OperandScale operand_scale = GetOperandScale(node);
   if (operand_scale != OperandScale::kSingle) {
     Bytecode prefix = Bytecodes::OperandScaleToPrefixBytecode(operand_scale);
-    bytecodes()->push_back(Bytecodes::ToByte(prefix));
+    *buffer_limit++ = Bytecodes::ToByte(prefix);
   }
 
   Bytecode bytecode = node->bytecode();
-  bytecodes()->push_back(Bytecodes::ToByte(bytecode));
+  *buffer_limit++ = Bytecodes::ToByte(bytecode);
 
-  int register_operand_bitmap = Bytecodes::GetRegisterOperandBitmap(bytecode);
   const uint32_t* const operands = node->operands();
-  const OperandSize* operand_sizes =
-      Bytecodes::GetOperandSizes(bytecode, operand_scale);
   const OperandType* operand_types = Bytecodes::GetOperandTypes(bytecode);
-  for (int i = 0; operand_types[i] != OperandType::kNone; ++i) {
-    OperandType operand_type = operand_types[i];
-    switch (operand_sizes[i]) {
+  const int operand_count = Bytecodes::NumberOfOperands(bytecode);
+  for (int i = 0; i < operand_count; ++i) {
+    OperandSize operand_size =
+        Bytecodes::SizeOfOperand(operand_types[i], operand_scale);
+    switch (operand_size) {
       case OperandSize::kNone:
         UNREACHABLE();
         break;
       case OperandSize::kByte:
-        bytecodes()->push_back(static_cast<uint8_t>(operands[i]));
+        *buffer_limit++ = static_cast<uint8_t>(operands[i]);
         break;
       case OperandSize::kShort: {
-        uint8_t operand_bytes[2];
-        WriteUnalignedUInt16(operand_bytes, operands[i]);
-        bytecodes()->insert(bytecodes()->end(), operand_bytes,
-                            operand_bytes + 2);
+        WriteUnalignedUInt16(buffer_limit, operands[i]);
+        buffer_limit += 2;
         break;
       }
       case OperandSize::kQuad: {
-        uint8_t operand_bytes[4];
-        WriteUnalignedUInt32(operand_bytes, operands[i]);
-        bytecodes()->insert(bytecodes()->end(), operand_bytes,
-                            operand_bytes + 4);
+        WriteUnalignedUInt32(buffer_limit, operands[i]);
+        buffer_limit += 4;
         break;
       }
     }
+  }
 
-    if ((register_operand_bitmap >> i) & 1) {
-      int count;
-      if (operand_types[i + 1] == OperandType::kRegCount) {
-        count = static_cast<int>(operands[i + 1]);
-      } else {
-        count = Bytecodes::GetNumberOfRegistersRepresentedBy(operand_type);
-      }
-      Register reg = Register::FromOperand(static_cast<int32_t>(operands[i]));
-      max_register_count_ = std::max(max_register_count_, reg.index() + count);
+  DCHECK_LE(buffer_limit, buffer + sizeof(buffer));
+  bytecodes()->insert(bytecodes()->end(), buffer, buffer_limit);
+
+  for (int i = 0; i < operand_count; ++i) {

[rmcilroy, 2016/06/27 14:03:48]

Could we keep doing this as part of the loop above? Any reason for the change of
approach here compared to the previous bitmap approach?

[oth, 2016/06/27 18:53:50]

Done.

It makes no difference either way perf wise. As you look at the code, it makes
two distinct blocks (candidate functions) for packing data and doing the maximum
register computation. No attachment to this change.

+    int count = Bytecodes::GetNumberOfRegistersRepresentedBy(operand_types[i]);
+    if (count == 0) {
+      continue;
+    }
+    uint32_t reg_operand = operands[i];
+    if (operand_types[i + 1] == OperandType::kRegCount) {

[rmcilroy, 2016/06/27 14:03:48]

I've been caught out again thinking this goes out-of-bounds. Could you add a
comment that the list is terminated by kNone so the size of the list is actually
one larger than the operand count (I find this pretty confusing TBH, but can
live with it for now).

[oth, 2016/06/27 18:53:50]

Done. It's no more or less confusing than having a terminator character in
C-strings. It's documented in bytecodes.h and now everywhere it's used. Fine
here if you want to remove it, but that's not something I am interested in doing
in this CL.

+      count = static_cast<int>(operands[++i]);
     }
+    Register reg = Register::FromOperand(static_cast<int32_t>(reg_operand));
+    max_register_count_ = std::max(max_register_count_, reg.index() + count);

[rmcilroy, 2016/06/27 14:03:48]

Any idea how much performance this part of the loop costs us? It seems like
something the register allocator should keep track of (with input from REO)
rather than us having to track each operand at this stage. Just a thought, not
blocking this CL.

[oth, 2016/06/27 18:53:50]

Three runs on my workstation suggest it is in the noise.

Not having this calculation: 1889, 2033, 1988.
Having this calculation: 1985, 2013, 1976.

I suspect it's cleaner to do it in the final stage than add plumbing between REO
and here to calculate this. It's also conceivable that another stage could
remove temporary registers (LDAR/STAR elimination in peephole optimizer could in
principle). The ground truth is what reaches the final stage.

   }
 }