[interpreter] Streamline bytecode array writing.

src/interpreter/bytecode-array-writer.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/interpreter/bytecode-array-writer.h"
 
 #include "src/api.h"
 #include "src/interpreter/bytecode-label.h"
 #include "src/interpreter/constant-array-builder.h"
 #include "src/log.h"
 
 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),
       bytecodes_(zone),
       max_register_count_(0),
       unbound_jumps_(0),
       source_position_table_builder_(isolate, zone),
       constant_array_builder_(constant_array_builder) {
   LOG_CODE_EVENT(isolate_, CodeStartLinePosInfoRecordEvent(
                                source_position_table_builder()));
@@ skipping 99 matching lines @@
 
 OperandScale OperandScaleForScalableUnsignedByte(uint32_t operand_value) {
   OperandSize bytes_required = Bytecodes::SizeForUnsignedOperand(operand_value);
   return ScaleForScalableByteOperand(bytes_required);
 }
 
 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);
         operand_scale =
             std::max(operand_scale, OperandScaleForScalableSignedByte(operand));
         break;
       }
       case OperandTypeInfo::kScalableUnsignedByte: {
         uint32_t operand = node->operand(i);
         operand_scale = std::max(operand_scale,
                                  OperandScaleForScalableUnsignedByte(operand));
         break;
       }
       case OperandTypeInfo::kFixedUnsignedByte:
       case OperandTypeInfo::kFixedUnsignedShort:
         break;
       case OperandTypeInfo::kNone:
         UNREACHABLE();
         break;
     }
   }
   return operand_scale;
 }
 
 }  // namespace
 
 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.

   }
 }
 
 // static
 Bytecode GetJumpWithConstantOperand(Bytecode jump_bytecode) {
   switch (jump_bytecode) {
     case Bytecode::kJump:
       return Bytecode::kJumpConstant;
     case Bytecode::kJumpIfTrue:
       return Bytecode::kJumpIfTrueConstant;
@@ skipping 157 matching lines @@
         node->set_bytecode(node->bytecode(), k32BitJumpPlaceholder);
         break;
     }
   }
   EmitBytecode(node);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8