[interpreter] Introduce fused bytecodes for common sequences.

src/interpreter/bytecode-peephole-optimizer.cc

Index: src/interpreter/bytecode-peephole-optimizer.cc
diff --git a/src/interpreter/bytecode-peephole-optimizer.cc b/src/interpreter/bytecode-peephole-optimizer.cc
index cff15f6f8050c93f1826832b7c187b3ed9a4f905..39a7c3a25a0f93f5dfd91ca77d5c6a2a78de7171 100644
--- a/src/interpreter/bytecode-peephole-optimizer.cc
+++ b/src/interpreter/bytecode-peephole-optimizer.cc
@@ -94,25 +94,8 @@ bool BytecodePeepholeOptimizer::LastBytecodePutsNameInAccumulator() const {
            GetConstantForIndexOperand(&last_, 0)->IsName()));
 }
 
-void BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes(
-    BytecodeNode* current) {
-  if (Bytecodes::IsJumpIfToBoolean(current->bytecode()) &&
-      Bytecodes::WritesBooleanToAccumulator(last_.bytecode())) {
-    // Conditional jumps with boolean conditions are emitted in
-    // ToBoolean form by the bytecode array builder,
-    // i.e. JumpIfToBooleanTrue rather JumpIfTrue. The ToBoolean element
-    // can be removed if the previous bytecode put a boolean value in
-    // the accumulator.
-    Bytecode jump = Bytecodes::GetJumpWithoutToBoolean(current->bytecode());
-    current->set_bytecode(jump, current->operand(0), current->operand_scale());
-  } else if (current->bytecode() == Bytecode::kToBooleanLogicalNot &&
-             Bytecodes::WritesBooleanToAccumulator(last_.bytecode())) {
-    // Logical-nots are emitted in ToBoolean form by the bytecode array
-    // builder, The ToBoolean element can be removed if the previous bytecode
-    // put a boolean value in the accumulator.
-    current->set_bytecode(Bytecode::kLogicalNot);
-  }
-
+void BytecodePeepholeOptimizer::TryToRemoveLastExpressionPosition(

[rmcilroy, 2016/05/24 10:53:56]

nit MaybeRemoveLastExpressionPosition

[oth, 2016/05/24 13:37:12]

IMO The prefix TryTo better fits what the function does than Maybe. Maybe
suggests a degree of arbitrariness on whether it occurs.

[rmcilroy, 2016/05/24 14:00:10]

Acknowledged.

+    const BytecodeNode* const current) {
   if (current->source_info().is_statement() &&
       last_.source_info().is_expression() &&
       Bytecodes::IsWithoutExternalSideEffects(last_.bytecode())) {
@@ -179,6 +162,77 @@ bool BytecodePeepholeOptimizer::CanElideLastBasedOnSourcePosition(
           !current->source_info().is_valid());
 }
 
+void BytecodePeepholeOptimizer::ChangeLoadStarToLdrLdar(
+    Bytecode new_bytecode, BytecodeNode* const current) {
+  last_.Transform(new_bytecode, current->operand(0), current->operand_scale());
+  current->set_bytecode(Bytecode::kLdar, current->operand(0),
+                        current->operand_scale());
+  if (current->source_info().is_valid()) {
+    last_.source_info().Update(current->source_info());

[rmcilroy, 2016/05/24 10:53:56]

nit - comment on why you move the source position backwards.

[oth, 2016/05/24 13:37:12]

Done.

+    current->source_info().set_invalid();
+  }
+}
+
+bool BytecodePeepholeOptimizer::ChangeLoadToLdr(BytecodeNode* const current) {
+  if (current->bytecode() == Bytecode::kStar) {
+    switch (last_.bytecode()) {
+      case Bytecode::kLoadIC:
+        ChangeLoadStarToLdrLdar(Bytecode::kLdrNamedProperty, current);
+        return true;
+      case Bytecode::kKeyedLoadIC:
+        ChangeLoadStarToLdrLdar(Bytecode::kLdrKeyedProperty, current);
+        return true;
+      case Bytecode::kLdaGlobal:
+        ChangeLoadStarToLdrLdar(Bytecode::kLdrGlobal, current);
+        return true;
+      case Bytecode::kLdaContextSlot:
+        ChangeLoadStarToLdrLdar(Bytecode::kLdrContextSlot, current);
+        return true;
+      case Bytecode::kLdaUndefined:
+        ChangeLoadStarToLdrLdar(Bytecode::kLdrUndefined, current);
+        return true;
+      default:
+        break;
+    }
+  }
+  return false;
+}
+
+bool BytecodePeepholeOptimizer::RemoveToBooleanFromJump(
+    BytecodeNode* const current) {
+  bool can_remove = Bytecodes::IsJumpIfToBoolean(current->bytecode()) &&
+                    Bytecodes::WritesBooleanToAccumulator(last_.bytecode());
+  if (can_remove) {
+    // Conditional jumps with boolean conditions are emiitted in
+    // ToBoolean form by the bytecode array builder,
+    // i.e. JumpIfToBooleanTrue rather JumpIfTrue. The ToBoolean
+    // element can be removed if the previous bytecode put a boolean
+    // value in the accumulator.
+    Bytecode jump = Bytecodes::GetJumpWithoutToBoolean(current->bytecode());
+    current->set_bytecode(jump, current->operand(0), current->operand_scale());
+  }
+  return can_remove;
+}
+
+bool BytecodePeepholeOptimizer::RemoveToBooleanFromLogicalNot(
+    BytecodeNode* const current) {
+  bool can_remove = current->bytecode() == Bytecode::kToBooleanLogicalNot &&
+                    Bytecodes::WritesBooleanToAccumulator(last_.bytecode());
+  if (can_remove) {
+    // Logical-nots are emitted in ToBoolean form by the bytecode array
+    // builder, The ToBoolean element can be removed if the previous bytecode
+    // put a boolean value in the accumulator.
+    current->set_bytecode(Bytecode::kLogicalNot);
+  }
+  return can_remove;
+}
+
+bool BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes(

[rmcilroy, 2016/05/24 10:53:56]

nit - s/UpdateLastAndCurrentBytecodes/CanTransformLastAndCurrent/ (or
CanChangeLastAndCurrent)

[oth, 2016/05/24 13:37:12]

The prefix 'Can' suggests testing a property of the pair, not actually
performing the update. The point of use will look confusing as there will be no
corresponding action for CanTransformLastAndCurrent() wherease the other Can
prefixed methods have associated actions in Optimize().

[rmcilroy, 2016/05/24 14:00:10]

OK, agree on "Can". I would still prefer Update is renamed to Transform or
Change though.

[oth, 2016/05/24 15:07:14]

Done.

+    BytecodeNode* const current) {
+  return RemoveToBooleanFromJump(current) ||
+         RemoveToBooleanFromLogicalNot(current) || ChangeLoadToLdr(current);
+}
+
 bool BytecodePeepholeOptimizer::CanElideLast(
     const BytecodeNode* const current) const {
   if (!last_is_discardable_) {
@@ -194,23 +248,32 @@ bool BytecodePeepholeOptimizer::CanElideLast(
     // consecutive accumulator loads (that don't have side effects) then only
     // the final load is potentially visible.
     return true;
+  } else if (Bytecodes::GetAccumulatorUse(current->bytecode()) ==
+                 AccumulatorUse::kWrite &&
+             Bytecodes::IsAccumulatorLoadWithoutEffects(last_.bytecode())) {
+    // The current instruction clobbers the accumulator without reading it. The
+    // load in the last instruction can be elided as it has no effect.
+    return true;
   } else {
     return false;
   }
 }
 
 BytecodeNode* BytecodePeepholeOptimizer::Optimize(BytecodeNode* current) {
-  UpdateLastAndCurrentBytecodes(current);
-  if (CanElideCurrent(current)) {
-    if (current->source_info().is_valid()) {
-      current->set_bytecode(Bytecode::kNop);
-    } else {
-      current = nullptr;
+  TryToRemoveLastExpressionPosition(current);
+
+  if (!UpdateLastAndCurrentBytecodes(current)) {

[rmcilroy, 2016/05/24 10:53:56]

nit - rather than if nesting, how about early returns, i.e.:

if (CanTransformLastAndCurrent(current)) {
  return current;
} 

if (CanElideCurrent(current)) {
  if (current->source_info().is_valid()) {
    current->set_bytecode(Bytecode::kNop);
  } else {
    current = nullptr;
  }
  return current;
}

if (CanElideLast(current) &&
    CanElideLastBasedOnSourcePosition(current)) {
  current->source_info().Update(last_.source_info());
  InvalidateLast();
  return current;
} 

return current;

[oth, 2016/05/24 13:37:12]

Done generally have a mild leaning towards single return (or guarded return) for
readability and maintenance risk. Maybe just an age thing :-)

[rmcilroy, 2016/05/24 14:00:10]

I agree in the general case, however I think this is cleaner to avoid the deeper
nesting. Thanks.

+    if (CanElideCurrent(current)) {
+      if (current->source_info().is_valid()) {
+        current->set_bytecode(Bytecode::kNop);
+      } else {
+        current = nullptr;
+      }
+    } else if (CanElideLast(current) &&
+               CanElideLastBasedOnSourcePosition(current)) {
+      current->source_info().Update(last_.source_info());
+      InvalidateLast();
     }
-  } else if (CanElideLast(current) &&
-             CanElideLastBasedOnSourcePosition(current)) {
-    current->source_info().Update(last_.source_info());
-    InvalidateLast();
   }
   return current;
 }