[interpreter] Inline Star on dispatch for some bytecodes

src/interpreter/interpreter-assembler.cc

Index: src/interpreter/interpreter-assembler.cc
diff --git a/src/interpreter/interpreter-assembler.cc b/src/interpreter/interpreter-assembler.cc
index ee5f8be27981f19b63ddf8eb3029729cb147a7fa..9829bec3eef710cd6432aff34889a7fb0f61273e 100644
--- a/src/interpreter/interpreter-assembler.cc
+++ b/src/interpreter/interpreter-assembler.cc
@@ -30,6 +30,7 @@ InterpreterAssembler::InterpreterAssembler(Isolate* isolate, Zone* zone,
                         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),
@@ -39,6 +40,8 @@ InterpreterAssembler::InterpreterAssembler(Isolate* isolate, Zone* zone,
       stack_pointer_before_call_(nullptr) {
   accumulator_.Bind(
       Parameter(InterpreterDispatchDescriptor::kAccumulatorParameter));
+  bytecode_offset_.Bind(
+      Parameter(InterpreterDispatchDescriptor::kBytecodeOffsetParameter));
   if (FLAG_trace_ignition) {
     TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry);
   }
@@ -83,7 +86,7 @@ void InterpreterAssembler::SetContext(Node* value) {
 }
 
 Node* InterpreterAssembler::BytecodeOffset() {
-  return Parameter(InterpreterDispatchDescriptor::kBytecodeOffsetParameter);
+  return bytecode_offset_.value();
 }
 
 Node* InterpreterAssembler::BytecodeArrayTaggedPointer() {
@@ -506,8 +509,12 @@ void InterpreterAssembler::UpdateInterruptBudget(Node* weight) {
                       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) {
@@ -516,6 +523,9 @@ Node* InterpreterAssembler::Advance(Node* delta) {
 
 Node* InterpreterAssembler::Jump(Node* delta) {
   UpdateInterruptBudget(delta);
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);
+  }
   return DispatchTo(Advance(delta));
 }
 
@@ -538,17 +548,80 @@ void InterpreterAssembler::JumpIfWordNotEqual(Node* lhs, Node* rhs,
   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.

   }
@@ -564,15 +637,14 @@ 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()};
@@ -588,8 +660,7 @@ void InterpreterAssembler::DispatchWide(OperandScale operand_scale) {
   //   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);
   }