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

src/interpreter/bytecode-array-builder.cc

Index: src/interpreter/bytecode-array-builder.cc
diff --git a/src/interpreter/bytecode-array-builder.cc b/src/interpreter/bytecode-array-builder.cc
index 594d8faa25e02dec182eae96e94f802eaa01dd58..e1222f9f399adff5c9e16b400c4c71eaf5e572c8 100644
--- a/src/interpreter/bytecode-array-builder.cc
+++ b/src/interpreter/bytecode-array-builder.cc
@@ -11,19 +11,23 @@ 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));
@@ -52,6 +56,7 @@ class BytecodeArrayBuilder::PreviousBytecodeHelper {
 
  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.

 };
 
 
@@ -580,6 +585,9 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::PopContext(Register context) {
 
 
 bool BytecodeArrayBuilder::NeedToBooleanCast() {
+  if (!LastBytecodeInSameBlock()) {
+    return true;
+  }
   PreviousBytecodeHelper previous_bytecode(*this);
   switch (previous_bytecode.GetBytecode()) {
     case Bytecode::kToBoolean:
@@ -600,8 +608,6 @@ bool BytecodeArrayBuilder::NeedToBooleanCast() {
     case Bytecode::kTestIn:
     case Bytecode::kForInDone:
       return false;
-    // Also handles the case where the previous bytecode was in a different
-    // block.
     default:
       return true;
   }
@@ -609,10 +615,14 @@ bool BytecodeArrayBuilder::NeedToBooleanCast() {
 
 
 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: {
@@ -643,17 +653,21 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::CastAccumulatorToJSObject() {
 
 
 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;
   }
@@ -1089,12 +1103,14 @@ bool BytecodeArrayBuilder::LastBytecodeInSameBlock() const {
 
 
 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;
 }