[interpreter] Filter expression positions at source.

src/interpreter/bytecode-array-builder.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-array-builder.h"
 
 #include "src/compiler.h"
 #include "src/interpreter/bytecode-array-writer.h"
 #include "src/interpreter/bytecode-label.h"
 #include "src/interpreter/bytecode-peephole-optimizer.h"
@@ skipping 63 matching lines @@
   DCHECK(!bytecode_generated_);
   bytecode_generated_ = true;
 
   Handle<FixedArray> handler_table = handler_table_builder()->ToHandlerTable();
   return pipeline_->ToBytecodeArray(fixed_register_count(), parameter_count(),
                                     handler_table);
 }
 
 void BytecodeArrayBuilder::AttachSourceInfo(BytecodeNode* node) {
   if (latest_source_info_.is_valid()) {
-    node->source_info().Update(latest_source_info_);
-    latest_source_info_.set_invalid();
+    if (latest_source_info_.is_statement() || !FLAG_ignition_filter_positions ||
+        !Bytecodes::IsWithoutExternalSideEffects(node->bytecode())) {

[rmcilroy, 2016/06/07 10:21:41]

Please pull out a CanFilterPosition function for clarity

[oth, 2016/06/08 09:19:41]

Added NeedExpressionPosition(Bytecode).

+      node->source_info() = latest_source_info_;
+      latest_source_info_.set_invalid();

[rmcilroy, 2016/06/07 10:21:41]

I think you should set_invalid unconditionally, otherwise you might attach it to
a later node.

[oth, 2016/06/08 09:19:41]

The goal is to defer writing the expression position until we discover it's
needed. If we invalidate it unconditionally, it can't be applied to where it's
needed later, e.g.

E > Star r3
    LdaGlobal ...

Should become:

    Star r3
E > LdaGlobal ...

Not:

    Star r3
    LdaGlobal ...

[rmcilroy, 2016/06/08 13:45:11]

I see, makes sense. Could you add a comment here as such.

[oth, 2016/06/08 15:04:21]

Done.

+    }
   }
 }
 
 void BytecodeArrayBuilder::Output(Bytecode bytecode) {
   BytecodeNode node(bytecode);
   AttachSourceInfo(&node);
   pipeline()->Write(&node);
 }
 
 void BytecodeArrayBuilder::OutputScaled(Bytecode bytecode,
@@ skipping 619 matching lines @@
   latest_source_info_.Update({return_position_, true});
 }
 
 void BytecodeArrayBuilder::SetStatementPosition(Statement* stmt) {
   if (stmt->position() == RelocInfo::kNoPosition) return;
   latest_source_info_.Update({stmt->position(), true});
 }
 
 void BytecodeArrayBuilder::SetExpressionPosition(Expression* expr) {
   if (expr->position() == RelocInfo::kNoPosition) return;
+  if (latest_source_info_.is_expression()) {
+    latest_source_info_.set_invalid();

[rmcilroy, 2016/06/07 10:21:41]

Why do you need to set this as invalid, won't Update  update latest_source_info_
appropriately if both are expressions (and if not, it should).

[oth, 2016/06/08 09:19:41]

Update() does not do the right thing. I don't want to tweak Update here as I'd
like to get rid of it entirely.

[rmcilroy, 2016/06/08 13:45:11]

I'm fine with getting rid of Update (I agree the logic in there is hard to make
sense of), however I'd like to ensure that we keep the logic for whether a
source position should overwrite another one be centralized in one place -
whether that means having a couple of additional functions in SourcePositionInfo
with different semantics or one which takes some additional parameters to
specify the situation.

For this CL, I'm fine with doing this, but please add a comment as to why it is
set invalid and a TODO with how you intend to deal with this when Update
disappears.

[oth, 2016/06/08 15:04:20]

Amen.
Done.

+  }
   latest_source_info_.Update({expr->position(), false});
 }
 
 void BytecodeArrayBuilder::SetExpressionAsStatementPosition(Expression* expr) {
   if (expr->position() == RelocInfo::kNoPosition) return;
   latest_source_info_.Update({expr->position(), true});
 }
 
 bool BytecodeArrayBuilder::TemporaryRegisterIsLive(Register reg) const {
   return temporary_register_allocator()->RegisterIsLive(reg);
@@ skipping 283 matching lines @@
 }
 
 uint32_t BytecodeArrayBuilder::UnsignedOperand(size_t value) {
   DCHECK_LE(value, kMaxUInt32);
   return static_cast<uint32_t>(value);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8