[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..9b127319abed29f5e7e1ec1e420964897b44e4d6 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, MachineRepresentation::kTagged),
       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() {
@@ -131,6 +134,13 @@ Node* InterpreterAssembler::StoreRegister(Node* value, Node* reg_index) {
                              RegisterFrameOffset(reg_index), value);
 }
 
+Node* InterpreterAssembler::StoreAccumulatorToRegister() {
+  DCHECK_EQ(bytecode_, Bytecode::kStar);
+  Node* reg_index = BytecodeOperandReg(0);
+  Node* accumulator = GetAccumulator();
+  return StoreRegister(accumulator, reg_index);
+}
+
 Node* InterpreterAssembler::NextRegister(Node* reg_index) {
   // Register indexes are negative, so the next index is minus one.
   return IntPtrAdd(reg_index, IntPtrConstant(-1));
@@ -506,8 +516,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) {
@@ -538,17 +552,65 @@ 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, MachineRepresentation::kTagged);

[rmcilroy, 2016/07/18 11:07:51]

The offset is not a tagged value, it is just a raw int. Making it kTagged would
cause the GC to treat it like a tagged pointer, which could cause crashes). This
should be a intptr, so MachineType::PointerRepresentation().

[klaasb, 2016/07/18 15:15:29]

Done.

+  Variable var_bytecode(this, MachineRepresentation::kWord32);

[rmcilroy, 2016/07/18 11:07:51]

kWord8, no?

[klaasb, 2016/07/18 15:15:29]

Done.

+  var_offset.Bind(target_offset);
+  var_bytecode.Bind(target_bytecode);
+
+  if (Bytecodes::IsStarLookahead(bytecode_, operand_scale_)) {
+    InlineDispatchToStar(var_bytecode, var_offset);
+  }
+  return DispatchToLoaded(var_bytecode.value(), var_offset.value());
+}
+
+void InterpreterAssembler::InlineDispatchToStar(Variable& target_bytecode,

[rmcilroy, 2016/07/18 11:07:51]

nit - StarDispatchLookahead

[klaasb, 2016/07/18 15:15:29]

Done.

+                                                Variable& target_offset) {
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeExit);

[rmcilroy, 2016/07/18 11:07:51]

You are now doing this twice for the same dispatch in the case where you don't
inline the Star dispatch (both here and in DispatchToBytecodeHandlerEntry). You
should probably do this in the branch.

[klaasb, 2016/07/18 15:15:29]

Done.

+  }
+  DCHECK_EQ(accumulator_use_, Bytecodes::GetAccumulatorUse(bytecode_));
+  bytecode_ = Bytecode::kStar;

[rmcilroy, 2016/07/18 11:07:51]

Could you split up the code which does the check for whether the next bytecode
is a Star, and have a separate function which performs the actual inline Star.
Could you make the bytecode_ set to Star only in this inline star function
(saving and restoring AccumulatorUse for the original bytecode).

[klaasb, 2016/07/18 15:15:29]

Yes, this assumed that a single Dispatch call was basically the last thing the
handler generation did. Currently none of the bytecodes with lookahead use a
second call to Dispatch.

I've refactored the "store"/inlining branch into a separate function together
with the bytecode_, bytecode_offset_ switching and trace call generation.
Dispatch will generate the same code multiple times - as it actually did before,
it now also generates the Star inlining at the same time.
We can just rewrite the handlers to make jumps to a single Dispatch (in another
CL?), or generate a Label for the dispatch part and Goto to that?

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

Let's not do this in this CL (see other comments). 

+  bytecode_offset_.Bind(target_offset.value());

[rmcilroy, 2016/07/18 11:07:51]

Not sure why this is needed?

[klaasb, 2016/07/18 15:15:29]

For BytecodeOffset() to have the correct value when calling
BytecodeOperandReg(0) and Advance() when inlining Star and building the next
dispatch.

Doesn't actually need to Bind() though, refactored it together with the rest.

+  accumulator_use_ = AccumulatorUse::kNone;
+  if (FLAG_trace_ignition) {
+    TraceBytecode(Runtime::kInterpreterTraceBytecodeEntry);

[rmcilroy, 2016/07/18 11:07:51]

ditto

[klaasb, 2016/07/18 15:15:29]

Done.

+  }
+
+  Variable* merged_vars[2] = {&target_offset, &target_bytecode};

[rmcilroy, 2016/07/18 11:07:51]

I don't think you need these to list the merged variables on the done path, they
should be auto-merged for you unless you do a backwards branch.

[klaasb, 2016/07/18 15:15:29]

Done.

+  Label store(this), done(this, 2, merged_vars);
+
+  Node* star_bytecode = Int32Constant(static_cast<int>(Bytecode::kStar));
+  Node* is_star = WordEqual(target_bytecode.value(), star_bytecode);
+  BranchIf(is_star, &store, &done);
+  Bind(&store);
+  {
+    StoreAccumulatorToRegister();
+    Node* next_bytecode_offset = Advance();
+    target_offset.Bind(next_bytecode_offset);
+    target_bytecode.Bind(LoadBytecode(next_bytecode_offset));
+    Goto(&done);
+  }
+  Bind(&done);
 }
 
 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 DispatchToLoaded(target_bytecode, new_bytecode_offset);
+}
+
+Node* InterpreterAssembler::DispatchToLoaded(Node* target_bytecode,

[rmcilroy, 2016/07/18 11:07:51]

nit - DispatchToBytecode

[klaasb, 2016/07/18 15:15:29]

Done.

+                                             Node* new_bytecode_offset) {
   if (kPointerSize == 8) {
     target_bytecode = ChangeUint32ToUint64(target_bytecode);
   }
-
   if (FLAG_trace_ignition_dispatches) {
     TraceBytecodeDispatch(target_bytecode);
   }
@@ -588,8 +650,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);
   }