[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 6b6589af41f544ecffea19a1615d6cdf88b0a811..e4105d326415bda213b705e1d85535149dc05c0e 100644
--- a/src/interpreter/bytecode-peephole-optimizer.cc
+++ b/src/interpreter/bytecode-peephole-optimizer.cc
@@ -94,21 +94,8 @@ bool BytecodePeepholeOptimizer::LastBytecodePutsNameInAccumulator() const {
            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)) {
@@ -128,6 +115,76 @@ bool BytecodePeepholeOptimizer::CanElideCurrent(
   }
 }
 
+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_) {
@@ -144,25 +201,35 @@ 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) {
-  UpdateCurrentBytecode(current);
-
-  if (CanElideCurrent(current)) {
-    if (current->source_info().is_valid()) {
-      current->set_bytecode(Bytecode::kNop);
-    } else {
-      current = nullptr;
+  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;
+      }
     }
-  } else if (CanElideLast(current)) {
-    if (last_.source_info().is_valid()) {
-      current->source_info().Update(last_.source_info());
+
+    if (CanElideLast(current)) {
+      if (last_.source_info().is_valid()) {
+        current->source_info().Update(last_.source_info());
+      }
+      InvalidateLast();
     }
-    InvalidateLast();
   }
   return current;
 }