[interpreter] Introduce fused bytecodes for common sequences.

src/interpreter/bytecode-peephole-optimizer.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-peephole-optimizer.h"
 
 #include "src/interpreter/constant-array-builder.h"
 #include "src/objects-inl.h"
 #include "src/objects.h"
 
@@ skipping 76 matching lines @@
 }
 
 bool BytecodePeepholeOptimizer::LastBytecodePutsNameInAccumulator() const {
   DCHECK(LastIsValid());
   return (last_.bytecode() == Bytecode::kTypeOf ||
           last_.bytecode() == Bytecode::kToName ||
           (last_.bytecode() == Bytecode::kLdaConstant &&
            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())) {
     // The last bytecode has been marked as expression. It has no
     // external effects so can't throw and the current bytecode is a
     // source position. Remove the expression position on the last
     // bytecode to open up potential peephole optimizations and to
     // save the memory and perf cost of storing the unneeded
     // expression position.
     last_.source_info().set_invalid();
@@ skipping 46 matching lines @@
   // The last bytecode can be elided for the MAYBE cases if the last
   // bytecode is known not to throw. If it throws, the system would
   // not have correct stack trace information. The appropriate check
   // for this would be Bytecodes::IsWithoutExternalSideEffects(), but
   // this is not performed here to keep the check simple.
   //
   return (!last_.source_info().is_valid() ||
           !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_) {
     return false;
   }
 
   if (last_.bytecode() == Bytecode::kNop) {
     // Nop are placeholders for holding source position information.
     return true;
   } else if (Bytecodes::IsAccumulatorLoadWithoutEffects(current->bytecode()) &&
              Bytecodes::IsAccumulatorLoadWithoutEffects(last_.bytecode())) {
     // The accumulator is invisible to the debugger. If there is a sequence of
     // 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;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8