[interpreter] Ensure optimizations preserve source positions.

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 155 matching lines @@
   //
   return (!last_.source_info().is_valid() ||
           !current->source_info().is_valid());
 }
 
 namespace {
 
 void TransformLdaStarToLdrLdar(Bytecode new_bytecode, BytecodeNode* const last,
                                BytecodeNode* const current) {
   DCHECK_EQ(current->bytecode(), Bytecode::kStar);
+  if (current->source_info().is_statement() &&
+      last->source_info().is_statement()) {

[rmcilroy, 2016/06/07 09:46:10]

As discussed offline, I think we need to not do this if
current->source_info().is_statement() (irrespective of last's source position).

[oth, 2016/06/07 13:46:17]

Yes, thanks for examining this. I agree.

                                        2nd Bytecode
                                           Star
                            Nothing        Expr         Statement

               Nothing       Yes           Yes             No

1st Bytecode
LdaGlobal      Expr          Yes           Yes             No


               Statement     Yes           Yes             No

+    // The transform won't maintain causal ordering for the debugger.
+    return;

[rmcilroy, 2016/06/07 09:46:11]

This returns here without doing anything, but then returns "true" in
"ChangeLdaToLdr". Could we move the check into ChangeLdaToLdr

As an alternative (possibly clearer), could we split out
TransformCurrentBytecode and TransformLastAndCurrentBytecodes as separate
top-level calls in optimize, then we could move the statement/statement check
into TransformLastAndCurrentBytecodes, since we probably shouldn't be messing
with both if both are statements. WDYT?

[oth, 2016/06/07 13:46:17]

Done.
Done, good idea. Since we only have the one transform in
TransformLastAndCurrent, the check is in the branch to do the transform since we
don't know if this rule would apply to other potential transforms.

+  }
+
   //
   // An example transformation here would be:
   //
   //   LdaGlobal i0, i1  ____\  LdrGlobal i0, i1, R
   //   Star R            ====/  Ldar R
   //
   // which loads a global value into both a register and the
   // accumulator. However, in the second form the Ldar can often be
   // peephole optimized away unlike the Star in the first form.
   //
   last->Transform(new_bytecode, current->operand(0), current->operand_scale());
   current->set_bytecode(Bytecode::kLdar, current->operand(0),
                         current->operand_scale());
 
   // If there was a source position on |current| transfer it to the
-  // updated |last| to maintain the debugger's causal view. ie. if an
-  // expression position LdrGlobal is the bytecode that could throw
-  // and if a statement position it needs to be placed before the
-  // store to R occurs.
-  last->source_info().Update(current->source_info());
-  current->source_info().set_invalid();
+  // updated |last| to maintain the debugger's causal view.
+  if (last->source_info().is_statement()) {
+    last->source_info().Update(current->source_info());
+    current->source_info().set_invalid();
+  }

[rmcilroy, 2016/06/07 09:46:11]

As discussed offline - I don't think this block does anything any longer, we
should probably just remove it.

[oth, 2016/06/07 13:46:17]

Done.

 }
 
 }  // namespace
 
 bool BytecodePeepholeOptimizer::ChangeLdaToLdr(BytecodeNode* const current) {
   if (current->bytecode() == Bytecode::kStar) {
     switch (last_.bytecode()) {
       case Bytecode::kLdaNamedProperty:
         TransformLdaStarToLdrLdar(Bytecode::kLdrNamedProperty, &last_, current);
         return true;
@@ skipping 110 matching lines @@
       next_stage_->Write(&last_);
       InvalidateLast();
     }
   }
   return current;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8