[Interpreter] Fixes PreviousBytecodeHelper to check if previous bytecode is in the same basic block.

src/interpreter/bytecode-array-builder.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-builder.h"
 
 namespace v8 {
 namespace internal {
 namespace interpreter {
 
 class BytecodeArrayBuilder::PreviousBytecodeHelper {
  public:
   explicit PreviousBytecodeHelper(const BytecodeArrayBuilder& array_builder)
-      : array_builder_(array_builder) {}
+      : array_builder_(array_builder) {
+    // This helper is expected to be instantiated only when the last bytecode is
+    // in the same basicblock.

[rmcilroy, 2015/12/08 13:43:02]

/s/basicblock/basic block/ throughout, and also in CL title and description. 

Also, please update the CLs title to be the same as the first line of the
description and add newlines in the description to keep it < 80 characters.

[mythria, 2015/12/08 14:53:29]

Done.

+    CHECK(array_builder_.LastBytecodeInSameBlock());

[rmcilroy, 2015/12/08 13:43:02]

DCHECK is fine here I think.

[mythria, 2015/12/08 14:53:29]

Done.

+  }
 
-  Bytecode GetBytecode() const {
-    // Returns the previous bytecode in the same basicblock. If there is none it
-    // returns Bytecode::kLast.
-    if (!array_builder_.LastBytecodeInSameBlock()) {
-      return Bytecode::kLast;
-    }
+  // Returns the previous bytecode in the same basicblock. Expects that the
+  // lastbytecode is in the same basicblock.

[rmcilroy, 2015/12/08 13:43:02]

/s/lastbytecode/last bytecode/. Actually, there is no need for the second
sentence since it is covered in the constructor.

[mythria, 2015/12/08 14:53:29]

Done.

+  MUST_USE_RESULT Bytecode GetBytecode() const {
+    DCHECK(array_builder_.LastBytecodeInSameBlock());

[rmcilroy, 2015/12/08 13:43:02]

Rather than DCHECKING this (which is already CHECKED above), could we instead
stash array_builder_.last_bytecode_start_ in the constructor, and DCHECK that it
is still equal here. This has the added benefit of ensuring that GetBytecode and
GetOperand always refer to the same bytecode for the time when this helper was
constructed.

[mythria, 2015/12/08 14:53:29]

Done.

     return Bytecodes::FromByte(
         array_builder_.bytecodes()->at(array_builder_.last_bytecode_start_));
   }
 
-  uint32_t GetOperand(int operand_index) const {
+  // Returns the operand at operand_index for the previous bytecode. Expects
+  // that the previous bytecode is in the same basicblock.

[rmcilroy, 2015/12/08 13:43:02]

ditto for the last sentence.

[mythria, 2015/12/08 14:53:29]

Done.

+  MUST_USE_RESULT uint32_t GetOperand(int operand_index) const {
     Bytecode bytecode = GetBytecode();
     DCHECK_GE(operand_index, 0);
     DCHECK_LT(operand_index, Bytecodes::NumberOfOperands(bytecode));
     size_t operand_offset =
         array_builder_.last_bytecode_start_ +
         Bytecodes::GetOperandOffset(bytecode, operand_index);
     OperandSize size = Bytecodes::GetOperandSize(bytecode, operand_index);
     switch (size) {
       default:
       case OperandSize::kNone:
         UNREACHABLE();
       case OperandSize::kByte:
         return static_cast<uint32_t>(
             array_builder_.bytecodes()->at(operand_offset));
       case OperandSize::kShort:
         uint16_t operand =
             (array_builder_.bytecodes()->at(operand_offset) << 8) +
             array_builder_.bytecodes()->at(operand_offset + 1);
         return static_cast<uint32_t>(operand);
     }
   }
 
   Handle<Object> GetConstantForIndexOperand(int operand_index) const {
     return array_builder_.constants_.at(GetOperand(operand_index));
   }
 
  private:
   const BytecodeArrayBuilder& array_builder_;
+  DISALLOW_COPY_AND_ASSIGN(PreviousBytecodeHelper);

[rmcilroy, 2015/12/08 13:43:02]

nit - newline above

[mythria, 2015/12/08 14:53:29]

Done.

 };
 
 
 BytecodeArrayBuilder::BytecodeArrayBuilder(Isolate* isolate, Zone* zone)
     : isolate_(isolate),
       zone_(zone),
       bytecodes_(zone),
       bytecode_generated_(false),
       last_block_end_(0),
       last_bytecode_start_(~0),
@@ skipping 508 matching lines @@
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::PopContext(Register context) {
   Output(Bytecode::kPopContext, context.ToOperand());
   return *this;
 }
 
 
 bool BytecodeArrayBuilder::NeedToBooleanCast() {
+  if (!LastBytecodeInSameBlock()) {
+    return true;
+  }
   PreviousBytecodeHelper previous_bytecode(*this);
   switch (previous_bytecode.GetBytecode()) {
     case Bytecode::kToBoolean:
       UNREACHABLE();
     // If the previous bytecode puts a boolean in the accumulator return true.
     case Bytecode::kLdaTrue:
     case Bytecode::kLdaFalse:
     case Bytecode::kLogicalNot:
     case Bytecode::kTestEqual:
     case Bytecode::kTestNotEqual:
     case Bytecode::kTestEqualStrict:
     case Bytecode::kTestNotEqualStrict:
     case Bytecode::kTestLessThan:
     case Bytecode::kTestLessThanOrEqual:
     case Bytecode::kTestGreaterThan:
     case Bytecode::kTestGreaterThanOrEqual:
     case Bytecode::kTestInstanceOf:
     case Bytecode::kTestIn:
     case Bytecode::kForInDone:
       return false;
-    // Also handles the case where the previous bytecode was in a different
-    // block.
     default:
       return true;
   }
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToBoolean() {
-  PreviousBytecodeHelper previous_bytecode(*this);
+  if (!LastBytecodeInSameBlock()) {
+    Output(Bytecode::kToBoolean);
+    return *this;
+  }
   // If the previous bytecode puts a boolean in the accumulator
   // there is no need to emit an instruction.
   if (NeedToBooleanCast()) {
+    PreviousBytecodeHelper previous_bytecode(*this);
     switch (previous_bytecode.GetBytecode()) {
       // If the previous bytecode is a constant evaluate it and return false.
       case Bytecode::kLdaZero: {
         LoadFalse();
         break;
       }
       case Bytecode::kLdaSmi8: {
         LoadBooleanConstant(previous_bytecode.GetOperand(0) != 0);
         break;
       }
@@ skipping 10 matching lines @@
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToJSObject() {
   Output(Bytecode::kToObject);
   return *this;
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToName() {
+  if (!LastBytecodeInSameBlock()) {

[rmcilroy, 2015/12/08 13:43:02]

If you reverse this (do the check for if (LastBytecodeInSameBlock() {
PreviousBytecode... }) you only need the call to Output(Bytecode::kToName) once.

[mythria, 2015/12/08 14:53:29]

Done.

+    Output(Bytecode::kToName);
+    return *this;
+  }
   PreviousBytecodeHelper previous_bytecode(*this);
   switch (previous_bytecode.GetBytecode()) {
+    case Bytecode::kToName:
+    case Bytecode::kTypeOf:
+      return *this;
     case Bytecode::kLdaConstantWide:
     case Bytecode::kLdaConstant: {
       Handle<Object> object = previous_bytecode.GetConstantForIndexOperand(0);
       if (object->IsName()) return *this;
       break;
     }
-    case Bytecode::kToName:
-    case Bytecode::kTypeOf:
-      return *this;
     default:
       break;
   }
   Output(Bytecode::kToName);
   return *this;
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToNumber() {
   // TODO(rmcilroy): consider omitting if the preceeding bytecode always returns
@@ skipping 415 matching lines @@
 }
 
 
 bool BytecodeArrayBuilder::LastBytecodeInSameBlock() const {
   return last_bytecode_start_ < bytecodes()->size() &&
          last_bytecode_start_ >= last_block_end_;
 }
 
 
 bool BytecodeArrayBuilder::IsRegisterInAccumulator(Register reg) {
+  if (!LastBytecodeInSameBlock()) {

[rmcilroy, 2015/12/08 13:43:02]

ditto (avoids double return false)

[mythria, 2015/12/08 14:53:28]

Done.

+    return false;
+  }
   PreviousBytecodeHelper previous_bytecode(*this);
-  if (previous_bytecode.GetBytecode() == Bytecode::kLdar ||
-      previous_bytecode.GetBytecode() == Bytecode::kStar) {
-    if (reg == Register::FromOperand(previous_bytecode.GetOperand(0))) {
-      return true;
-    }
+  Bytecode bytecode = previous_bytecode.GetBytecode();
+  if ((bytecode == Bytecode::kLdar || bytecode == Bytecode::kStar) &&
+      (reg == Register::FromOperand(previous_bytecode.GetOperand(0)))) {
+    return true;
   }
   return false;
 }
 
 
 // static
 Bytecode BytecodeArrayBuilder::BytecodeForBinaryOperation(Token::Value op) {
   switch (op) {
     case Token::Value::ADD:
       return Bytecode::kAdd;
@@ skipping 316 matching lines @@
   DCHECK_GT(next_consecutive_count_, 0);
   builder_->BorrowConsecutiveTemporaryRegister(next_consecutive_register_);
   allocated_.push_back(next_consecutive_register_);
   next_consecutive_count_--;
   return Register(next_consecutive_register_++);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8