[interpreter] Inline Star on dispatch for some bytecodes

src/interpreter/interpreter-assembler.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/interpreter-assembler.h"
 
 #include <limits>
 #include <ostream>
 
 #include "src/code-factory.h"
@@ skipping 12 matching lines @@
 using compiler::Node;
 
 InterpreterAssembler::InterpreterAssembler(Isolate* isolate, Zone* zone,
                                            Bytecode bytecode,
                                            OperandScale operand_scale)
     : CodeStubAssembler(isolate, zone, InterpreterDispatchDescriptor(isolate),
                         Code::ComputeFlags(Code::BYTECODE_HANDLER),
                         Bytecodes::ToString(bytecode),
                         Bytecodes::ReturnCount(bytecode)),
       bytecode_(bytecode),
+      bytecode_offset_(this, MachineType::PointerRepresentation()),
       operand_scale_(operand_scale),
       interpreted_frame_pointer_(this, MachineType::PointerRepresentation()),
       accumulator_(this, MachineRepresentation::kTagged),
       accumulator_use_(AccumulatorUse::kNone),
       made_call_(false),
       disable_stack_check_across_call_(false),
       stack_pointer_before_call_(nullptr) {
   accumulator_.Bind(
       Parameter(InterpreterDispatchDescriptor::kAccumulatorParameter));
+  bytecode_offset_.Bind(
+      Parameter(InterpreterDispatchDescriptor::kBytecodeOffsetParameter));
   if (FLAG_trace_ignition) {
     TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry);
   }
 }
 
 InterpreterAssembler::~InterpreterAssembler() {
   // If the following check fails the handler does not use the
   // accumulator in the way described in the bytecode definitions in
   // bytecodes.h.
   DCHECK_EQ(accumulator_use_, Bytecodes::GetAccumulatorUse(bytecode_));
@@ skipping 24 matching lines @@
 
 Node* InterpreterAssembler::GetContext() {
   return LoadRegister(Register::current_context());
 }
 
 void InterpreterAssembler::SetContext(Node* value) {
   StoreRegister(value, Register::current_context());
 }
 
 Node* InterpreterAssembler::BytecodeOffset() {
-  return Parameter(InterpreterDispatchDescriptor::kBytecodeOffsetParameter);
+  return bytecode_offset_.value();
 }
 
 Node* InterpreterAssembler::BytecodeArrayTaggedPointer() {
   if (made_call_) {
     // If we have made a call, restore bytecode array from stack frame in case
     // the debugger has swapped us to the patched debugger bytecode array.
     return LoadRegister(Register::bytecode_array());
   } else {
     return Parameter(InterpreterDispatchDescriptor::kBytecodeArrayParameter);
   }
@@ skipping 402 matching lines @@
     Goto(&ok);
   }
 
   // Update budget.
   Bind(&ok);
   StoreNoWriteBarrier(MachineRepresentation::kWord32,
                       BytecodeArrayTaggedPointer(), budget_offset,
                       new_budget.value());
 }
 
+Node* InterpreterAssembler::Advance() {
+  return Advance(Bytecodes::Size(bytecode_, operand_scale_));
+}
+
 Node* InterpreterAssembler::Advance(int delta) {
-  return IntPtrAdd(BytecodeOffset(), IntPtrConstant(delta));
+  return Advance(IntPtrConstant(delta));
 }
 
 Node* InterpreterAssembler::Advance(Node* delta) {
   return IntPtrAdd(BytecodeOffset(), delta);
 }
 
 Node* InterpreterAssembler::Jump(Node* delta) {
   UpdateInterruptBudget(delta);
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);
+  }
   return DispatchTo(Advance(delta));
 }
 
 void InterpreterAssembler::JumpConditional(Node* condition, Node* delta) {
   Label match(this), no_match(this);
 
   BranchIf(condition, &match, &no_match);
   Bind(&match);
   Jump(delta);
   Bind(&no_match);
   Dispatch();
 }
 
 void InterpreterAssembler::JumpIfWordEqual(Node* lhs, Node* rhs, Node* delta) {
   JumpConditional(WordEqual(lhs, rhs), delta);
 }
 
 void InterpreterAssembler::JumpIfWordNotEqual(Node* lhs, Node* rhs,
                                               Node* delta) {
   JumpConditional(WordNotEqual(lhs, rhs), delta);
 }
 
+Node* InterpreterAssembler::LoadBytecode(compiler::Node* bytecode_offset) {
+  return Load(MachineType::Uint8(), BytecodeArrayTaggedPointer(),
+              bytecode_offset);
+}
+
 Node* InterpreterAssembler::Dispatch() {
-  return DispatchTo(Advance(Bytecodes::Size(bytecode_, operand_scale_)));
+  Node* target_offset = Advance();
+  Node* target_bytecode = LoadBytecode(target_offset);
+  Variable var_offset(this, MachineType::PointerRepresentation());
+  Variable var_bytecode(this, MachineRepresentation::kWord8);
+  var_offset.Bind(target_offset);
+  var_bytecode.Bind(target_bytecode);
+
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);

[rmcilroy, 2016/07/19 10:03:56]

I think the TraceBytecode can still stay in DispatchToBytecodeHandler. All you
need to do is add another TraceBytecode(Runtime::kInterpreterTraceBytecodeExit)
in StarDispatchLookahead (when exiting the current bytecode), then have the
TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry) in InlineStar and depend
on the exit in DispatchToBytecodeHandler for tracing the exit of Star.

Note: The TraceBytecode relies on BytecodeOffset being correct, so you will need
to update that in Advance() as mentioned above.

[klaasb, 2016/07/19 14:24:13]

I had it that way before, but probably the offset was incorrect at some point
and it printed the wrong acc/reg usage.
It's somehow still wrong after doing the Advance - there's something I'm not
understanding.

[rmcilroy, 2016/07/19 15:26:04]

As discussed offline, this is probably because you call Advance before
TraceBytecodeExit, which makes BytecodeOffset wrong for TraceBytecodeExit. Maybe
we could just move TraceBytecodeExit before (or into) the Advance? 

[klaasb, 2016/07/19 15:39:00]

Yes, that's what I did now.

+  }
+
+  if (Bytecodes::IsStarLookahead(bytecode_, operand_scale_)) {
+    StarDispatchLookahead(var_bytecode, var_offset);
+  }
+  return DispatchToBytecode(var_bytecode.value(), var_offset.value());
+}
+
+void InterpreterAssembler::StarDispatchLookahead(Variable& target_bytecode,
+                                                 Variable& target_offset) {
+  Label store(this), done(this);
+
+  Node* star_bytecode = Int32Constant(static_cast<int>(Bytecode::kStar));

[rmcilroy, 2016/07/19 10:03:56]

IntPtrConstant

[klaasb, 2016/07/19 14:24:13]

Done.

+  Node* is_star = WordEqual(target_bytecode.value(), star_bytecode);
+  BranchIf(is_star, &store, &done);
+  Bind(&store);

[rmcilroy, 2016/07/19 10:03:56]

nit - do_inline_star

[klaasb, 2016/07/19 14:24:13]

Done.

+  {
+    InlineStar(target_offset);
+    target_bytecode.Bind(LoadBytecode(target_offset.value()));
+    Goto(&done);
+  }
+  Bind(&done);
+}
+
+void InterpreterAssembler::InlineStar(Variable& target_offset) {
+  Bytecode previous_bytecode = bytecode_;
+  Variable previous_offset = bytecode_offset_;
+  AccumulatorUse previous_acc_use = accumulator_use_;
+  bytecode_ = Bytecode::kStar;

[rmcilroy, 2016/07/19 10:03:56]

nit - newline

[klaasb, 2016/07/19 14:24:13]

Done.

+  bytecode_offset_ = target_offset;
+  accumulator_use_ = AccumulatorUse::kNone;
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry);
+  }
+
+  StoreRegister(GetAccumulator(), BytecodeOperandReg(0));
+
+  DCHECK_EQ(accumulator_use_, Bytecodes::GetAccumulatorUse(bytecode_));
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);
+  }
+  Node* next_bytecode_offset = Advance();
+  target_offset.Bind(next_bytecode_offset);
+
+  bytecode_ = previous_bytecode;
+  bytecode_offset_ = previous_offset;
+  accumulator_use_ = previous_acc_use;
 }
 
 Node* InterpreterAssembler::DispatchTo(Node* new_bytecode_offset) {
-  Node* target_bytecode = Load(
-      MachineType::Uint8(), BytecodeArrayTaggedPointer(), new_bytecode_offset);
+  Node* target_bytecode = LoadBytecode(new_bytecode_offset);
+  return DispatchToBytecode(target_bytecode, new_bytecode_offset);
+}
+
+Node* InterpreterAssembler::DispatchToBytecode(Node* target_bytecode,
+                                               Node* new_bytecode_offset) {
   if (kPointerSize == 8) {
     target_bytecode = ChangeUint32ToUint64(target_bytecode);
   }

[rmcilroy, 2016/07/19 10:03:56]

This should also be in LoadBytecode (hopefully we can teach TF to avoid actually
emitting any code for this on most platforms, but we should be consistent and
have the bytecode always treated as word-sized (since it needs to be for the
load below).

[klaasb, 2016/07/19 14:24:13]

Done.

-
   if (FLAG_trace_ignition_dispatches) {
     TraceBytecodeDispatch(target_bytecode);

[rmcilroy, 2016/07/19 10:03:56]

Just to point out, we will no longer count these inlined Star's as bytecode
dispatches. This is probably what we want, but we should keep it in mind when
looking at dispatch count reports in the future.

[klaasb, 2016/07/19 14:24:13]

Acknowledged.

   }
 
   Node* target_code_entry =
       Load(MachineType::Pointer(), DispatchTableRawPointer(),
            WordShl(target_bytecode, IntPtrConstant(kPointerSizeLog2)));
 
   return DispatchToBytecodeHandlerEntry(target_code_entry, new_bytecode_offset);
 }
 
 Node* InterpreterAssembler::DispatchToBytecodeHandler(Node* handler,
                                                       Node* bytecode_offset) {
   Node* handler_entry =
       IntPtrAdd(handler, IntPtrConstant(Code::kHeaderSize - kHeapObjectTag));
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);
+  }
   return DispatchToBytecodeHandlerEntry(handler_entry, bytecode_offset);
 }
 
 Node* InterpreterAssembler::DispatchToBytecodeHandlerEntry(
     Node* handler_entry, Node* bytecode_offset) {
-  if (FLAG_trace_ignition) {
-    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);
-  }
-
   InterpreterDispatchDescriptor descriptor(isolate());
   Node* args[] = {GetAccumulatorUnchecked(), bytecode_offset,
                   BytecodeArrayTaggedPointer(), DispatchTableRawPointer()};
   return TailCallBytecodeDispatch(descriptor, handler_entry, args);
 }
 
 void InterpreterAssembler::DispatchWide(OperandScale operand_scale) {
   // Dispatching a wide bytecode requires treating the prefix
   // bytecode a base pointer into the dispatch table and dispatching
   // the bytecode that follows relative to this base.
   //
   //   Indices 0-255 correspond to bytecodes with operand_scale == 0
   //   Indices 256-511 correspond to bytecodes with operand_scale == 1
   //   Indices 512-767 correspond to bytecodes with operand_scale == 2
   Node* next_bytecode_offset = Advance(1);
-  Node* next_bytecode = Load(MachineType::Uint8(), BytecodeArrayTaggedPointer(),
-                             next_bytecode_offset);
+  Node* next_bytecode = LoadBytecode(next_bytecode_offset);
   if (kPointerSize == 8) {
     next_bytecode = ChangeUint32ToUint64(next_bytecode);
   }
 
   if (FLAG_trace_ignition_dispatches) {
     TraceBytecodeDispatch(next_bytecode);
   }
 
   Node* base_index;
   switch (operand_scale) {
@@ skipping 176 matching lines @@
     Goto(&loop);
   }
   Bind(&done_loop);
 
   return array;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8