[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::UpdateCurrentBytecode(BytecodeNode* current) {
-  // 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.
-  if (Bytecodes::IsJumpIfToBoolean(current->bytecode()) &&
-      Bytecodes::WritesBooleanToAccumulator(last_.bytecode())) {
-    Bytecode jump = Bytecodes::GetJumpWithoutToBoolean(current->bytecode());
-    current->set_bytecode(jump, current->operand(0), current->operand_scale());
-  }
-}
-
 bool BytecodePeepholeOptimizer::CanElideCurrent(
-    const BytecodeNode* const current) const {

[rmcilroy, 2016/05/17 15:49:33]

Still const?

[oth, 2016/05/18 20:22:25]

Done. Oops, lost const when it incorporated the change subsequently proposed in
this review for CanElideCurrent().

+    const BytecodeNode* const current) {
   if (Bytecodes::IsLdarOrStar(last_.bytecode()) &&
       Bytecodes::IsLdarOrStar(current->bytecode()) &&
       current->operand(0) == last_.operand(0)) {
     // Ldar and Star make the accumulator and register hold equivalent
     // values. Only the first bytecode is needed if there's a sequence
     // of back-to-back Ldar and Star bytecodes with the same operand.
     return true;
   } else if (current->bytecode() == Bytecode::kToName &&
              LastBytecodePutsNameInAccumulator()) {
     // If the previous bytecode ensured a name was in the accumulator,
     // the type coercion ToName() can be elided.
     return true;
   } else {
     // Additional candidates for eliding current:
     // (i) ToNumber if the last puts a number in the accumulator.
     return false;
   }
 }
 
+bool BytecodePeepholeOptimizer::ChangeLdarMovToLdarStar(

[rmcilroy, 2016/05/17 15:49:33]

This seems unrelated to the other optimizations. Can we land it in a different
CL?

[oth, 2016/05/18 20:22:25]

Done.

+    BytecodeNode* const current) {
+  bool can_simplify = last_.bytecode() == Bytecode::kLdar &&
+                      current->bytecode() == Bytecode::kMov &&
+                      last_.operand(0) == current->operand(0);
+  if (can_simplify) {
+    current->set_bytecode(Bytecode::kStar, current->operand(1),
+                          current->operand_scale());
+  }
+  return can_simplify;
+}
+
+void BytecodePeepholeOptimizer::RewriteLastLoadAndStar(

[rmcilroy, 2016/05/17 15:49:33]

The name confuses me. How about RewriteLdaToLdr.

[oth, 2016/05/18 20:22:25]

Changed to ChangeLoadStarToLdrLdar() which describes the transformation. 

+    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());
+    current->source_info().set_invalid();
+  }
+}
+
+bool BytecodePeepholeOptimizer::ChangeLdaToLdr(BytecodeNode* const current) {
+  if (current->bytecode() == Bytecode::kStar) {
+    switch (last_.bytecode()) {
+      case Bytecode::kLdaNamedProperty:
+        RewriteLastLoadAndStar(Bytecode::kLdrNamedProperty, current);
+        return true;
+      case Bytecode::kLdaKeyedProperty:
+        RewriteLastLoadAndStar(Bytecode::kLdrKeyedProperty, current);
+        return true;
+      case Bytecode::kLdaGlobal:
+        RewriteLastLoadAndStar(Bytecode::kLdrGlobal, current);
+        return true;
+      case Bytecode::kLdaContextSlot:
+        RewriteLastLoadAndStar(Bytecode::kLdrContextSlot, current);
+        return true;
+      case Bytecode::kLdaUndefined:
+        RewriteLastLoadAndStar(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::SimpleSubstitution(
+    BytecodeNode* const current) {
+  return RemoveToBooleanFromJump(current) || ChangeLdaToLdr(current) ||

[rmcilroy, 2016/05/17 15:49:33]

I think ChangeLdaToLdr should really be in CanElideCurrent - since in-effect it
is updating last to elide the current. I would prefer this (even if it means
renaming CanElideCurrent somehow) since it keeps the logic of updating the
source position when eliding the current instruction in one place. WDYT?

[oth, 2016/05/18 20:22:25]

This was actually implemented during the construction of this CL and rejected.

CanElideCurrent() is a test for the elision of the incoming bytecode. The name
does not suggest producing side effects of re-writing bytecodes.

The SimpleSubstution is either replacing the current or replacing both the
current and last. In neither case is eliding the current node appropriate.

The Optimize method is pretty clear as is - one of three things is performed,
direct bytecode substitution, eliding the current, or eliding the last.

[rmcilroy, 2016/05/19 10:01:02]

Acknowledged.

+         ChangeLdarMovToLdarStar(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
     // and can be elided.
     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) {
-  UpdateCurrentBytecode(current);
+  if (!SimpleSubstitution(current)) {

[rmcilroy, 2016/05/17 15:49:33]

Why the change in behaviour here? I think it makes sense to do
SimpleSubstitution then do CanElideCurrent irrespective of whether we
substituted it, in case there ends up being a elide optimization can happen even
after a substitution. WDYT?

[oth, 2016/05/18 20:22:25]

As discussed above, this is not appealing. There is no elision in
SimpleSubstitution (clearly a mutating method) vs CanElideCurrent (a
mutation-effect free test).

If there is a possible optimization after the simple substitution then simple
substitution was not the appropriate path and it will be costly in terms of
performance to perform multiple rounds of rule evaluation. If there are cases in
future that actually need this, let's make the change. In the interim, YAGNI.

Pending perf, the table based approach makes this much easier.

[rmcilroy, 2016/05/19 10:01:02]

Acknowledged.

+    if (CanElideCurrent(current)) {
+      if (current->source_info().is_valid()) {
+        if (current->source_info().is_statement()) {
+          current->set_bytecode(Bytecode::kNop);
+        }
+      } else {
+        return nullptr;
+      }
+    }
 
-  if (CanElideCurrent(current)) {
-    if (current->source_info().is_valid()) {
-      current->set_bytecode(Bytecode::kNop);
-    } else {
-      current = nullptr;
+    if (CanElideLast(current)) {
+      if (last_.source_info().is_valid()) {
+        current->source_info().Update(last_.source_info());
+      }
+      InvalidateLast();
     }
-  } else if (CanElideLast(current)) {
-    if (last_.source_info().is_valid()) {
-      current->source_info().Update(last_.source_info());
-    }
-    InvalidateLast();
   }
   return current;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8