 Chromium Code Reviews
 Chromium Code Reviews Issue 2684993002:
  [interpreter] Create custom call opcodes for specific argument counts  (Closed)
    
  
    Issue 2684993002:
  [interpreter] Create custom call opcodes for specific argument counts  (Closed) 
  | Index: src/interpreter/interpreter.cc | 
| diff --git a/src/interpreter/interpreter.cc b/src/interpreter/interpreter.cc | 
| index 3b4bb5fc8a0e760a0fc60d320f2a6821d7d9280a..8002ff2a26968c3101fcb27bd9a45bfbe6804d2b 100644 | 
| --- a/src/interpreter/interpreter.cc | 
| +++ b/src/interpreter/interpreter.cc | 
| @@ -4,6 +4,7 @@ | 
| #include "src/interpreter/interpreter.h" | 
| +#include <array> | 
| #include <fstream> | 
| #include <memory> | 
| @@ -134,23 +135,49 @@ void Interpreter::InstallBytecodeHandler(Zone* zone, Bytecode bytecode, | 
| BytecodeGeneratorFunc generator) { | 
| if (!Bytecodes::BytecodeHasHandler(bytecode, operand_scale)) return; | 
| - InterpreterDispatchDescriptor descriptor(isolate_); | 
| - compiler::CodeAssemblerState state( | 
| - isolate_, zone, descriptor, Code::ComputeFlags(Code::BYTECODE_HANDLER), | 
| - Bytecodes::ToString(bytecode), Bytecodes::ReturnCount(bytecode)); | 
| - InterpreterAssembler assembler(&state, bytecode, operand_scale); | 
| - if (Bytecodes::MakesCallAlongCriticalPath(bytecode)) { | 
| - assembler.SaveBytecodeOffset(); | 
| - } | 
| - (this->*generator)(&assembler); | 
| - Handle<Code> code = compiler::CodeAssembler::GenerateCode(&state); | 
| size_t index = GetDispatchTableIndex(bytecode, operand_scale); | 
| - dispatch_table_[index] = code->entry(); | 
| - TraceCodegen(code); | 
| - PROFILE(isolate_, CodeCreateEvent( | 
| - CodeEventListener::BYTECODE_HANDLER_TAG, | 
| - AbstractCode::cast(*code), | 
| - Bytecodes::ToString(bytecode, operand_scale).c_str())); | 
| + switch (bytecode) { | 
| + case Bytecode::kCallProperty: | 
| + case Bytecode::kCallProperty0: | 
| + case Bytecode::kCallProperty1: | 
| + case Bytecode::kCallProperty2: { | 
| + const int offset = static_cast<int>(Bytecode::kCallProperty) - | 
| + static_cast<int>(Bytecode::kCall); | 
| + STATIC_ASSERT(offset == | 
| + static_cast<int>(Bytecode::kCallProperty0) - | 
| + static_cast<int>(Bytecode::kCall0)); | 
| + STATIC_ASSERT(offset == | 
| + static_cast<int>(Bytecode::kCallProperty1) - | 
| + static_cast<int>(Bytecode::kCall1)); | 
| + STATIC_ASSERT(offset == | 
| + static_cast<int>(Bytecode::kCallProperty2) - | 
| + static_cast<int>(Bytecode::kCall2)); | 
| 
rmcilroy
2017/02/17 11:37:19
Can you also DCHECK that offset < index
 
danno
2017/02/17 17:18:43
Done.
 | 
| + dispatch_table_[index] = dispatch_table_[index - offset]; | 
| + break; | 
| + } | 
| 
rmcilroy
2017/02/17 11:37:19
Could we pull the duplication logic out to a separ
 
danno
2017/02/17 17:18:43
Done.
 | 
| + | 
| + default: { | 
| + InterpreterDispatchDescriptor descriptor(isolate_); | 
| + compiler::CodeAssemblerState state( | 
| + isolate_, zone, descriptor, | 
| + Code::ComputeFlags(Code::BYTECODE_HANDLER), | 
| + Bytecodes::ToString(bytecode), Bytecodes::ReturnCount(bytecode)); | 
| + InterpreterAssembler assembler(&state, bytecode, operand_scale); | 
| + if (Bytecodes::MakesCallAlongCriticalPath(bytecode)) { | 
| + assembler.SaveBytecodeOffset(); | 
| + } | 
| + (this->*generator)(&assembler); | 
| + Handle<Code> code = compiler::CodeAssembler::GenerateCode(&state); | 
| + dispatch_table_[index] = code->entry(); | 
| + TraceCodegen(code); | 
| + PROFILE(isolate_, | 
| + CodeCreateEvent( | 
| + CodeEventListener::BYTECODE_HANDLER_TAG, | 
| + AbstractCode::cast(*code), | 
| + Bytecodes::ToString(bytecode, operand_scale).c_str())); | 
| + break; | 
| + } | 
| + } | 
| } | 
| Code* Interpreter::GetBytecodeHandler(Bytecode bytecode, | 
| @@ -2151,6 +2178,33 @@ void Interpreter::DoJSCall(InterpreterAssembler* assembler, | 
| __ Dispatch(); | 
| } | 
| +void Interpreter::DoJSCallN(InterpreterAssembler* assembler, int n) { | 
| 
rmcilroy
2017/02/17 11:37:19
nit - n -> arg_count
 
danno
2017/02/17 17:18:43
Done.
 | 
| + const int kFunctionOperandCount = 1; | 
| + const int kReceiverOperandCount = 1; | 
| + const int kBoilerplatParameterCount = 7; | 
| + const int kReceiveParameterOffset = 5; | 
| 
rmcilroy
2017/02/17 11:37:19
nit - receiverParameterOffset
 
danno
2017/02/17 17:18:43
Done.
 | 
| + | 
| + Node* function_reg = __ BytecodeOperandReg(0); | 
| + Node* function = __ LoadRegister(function_reg); | 
| + std::array<Node*, Bytecodes::kMaxOperands + kBoilerplatParameterCount> temp; | 
| + for (int i = 0; i < (n + kReceiverOperandCount); ++i) { | 
| 
rmcilroy
2017/02/17 11:37:19
nit - could you do these in-order (the for loop be
 
danno
2017/02/17 17:18:43
Done.
 | 
| + Node* reg = __ BytecodeOperandReg(i + kFunctionOperandCount); | 
| 
rmcilroy
2017/02/17 11:37:19
nit - I find the use of kFunctionOperandCount and
 
danno
2017/02/17 17:18:43
Done.
 | 
| + temp[i + kReceiveParameterOffset] = __ LoadRegister(reg); | 
| + } | 
| + Callable call_ic = CodeFactory::CallIC(isolate_); | 
| + temp[0] = __ HeapConstant(call_ic.code()); | 
| + temp[1] = function; | 
| + temp[2] = __ Int32Constant(n); | 
| + temp[3] = __ BytecodeOperandIdxInt32(n + kFunctionOperandCount + | 
| + kReceiverOperandCount); | 
| + temp[4] = __ LoadFeedbackVector(); | 
| + temp[n + 6] = __ GetContext(); | 
| 
rmcilroy
2017/02/17 11:37:19
nit - temp[5 + kRecieverCount + n]
 
danno
2017/02/17 17:18:43
Done.
 | 
| + Node* result = __ CallStubN(call_ic.descriptor(), 1, | 
| + n + kBoilerplatParameterCount, &temp[0]); | 
| + __ SetAccumulator(result); | 
| + __ Dispatch(); | 
| +} | 
| + | 
| // Call <callable> <receiver> <arg_count> <feedback_slot_id> | 
| // | 
| // Call a JSfunction or Callable in |callable| with the |receiver| and | 
| @@ -2160,15 +2214,36 @@ void Interpreter::DoCall(InterpreterAssembler* assembler) { | 
| DoJSCall(assembler, TailCallMode::kDisallow); | 
| } | 
| -// CallProperty <callable> <receiver> <arg_count> <feedback_slot_id> | 
| -// | 
| -// Call a JSfunction or Callable in |callable| with the |receiver| and | 
| -// |arg_count| arguments in subsequent registers. Collect type feedback into | 
| -// |feedback_slot_id|. The callable is known to be a property of the receiver. | 
| +void Interpreter::DoCall0(InterpreterAssembler* assembler) { | 
| + DoJSCallN(assembler, 0); | 
| +} | 
| + | 
| +void Interpreter::DoCall1(InterpreterAssembler* assembler) { | 
| + DoJSCallN(assembler, 1); | 
| +} | 
| + | 
| +void Interpreter::DoCall2(InterpreterAssembler* assembler) { | 
| + DoJSCallN(assembler, 2); | 
| +} | 
| + | 
| void Interpreter::DoCallProperty(InterpreterAssembler* assembler) { | 
| - // TODO(leszeks): Look into making the interpreter use the fact that the | 
| - // receiver is non-null. | 
| - DoJSCall(assembler, TailCallMode::kDisallow); | 
| + // Same as Call | 
| + UNREACHABLE(); | 
| +} | 
| + | 
| +void Interpreter::DoCallProperty0(InterpreterAssembler* assembler) { | 
| + // Same as Call0 | 
| + UNREACHABLE(); | 
| +} | 
| + | 
| +void Interpreter::DoCallProperty1(InterpreterAssembler* assembler) { | 
| + // Same as Call1 | 
| + UNREACHABLE(); | 
| +} | 
| + | 
| +void Interpreter::DoCallProperty2(InterpreterAssembler* assembler) { | 
| + // Same as Call2 | 
| + UNREACHABLE(); | 
| } | 
| // TailCall <callable> <receiver> <arg_count> <feedback_slot_id> | 
| @@ -2228,7 +2303,6 @@ void Interpreter::DoCallRuntimeForPair(InterpreterAssembler* assembler) { | 
| Node* context = __ GetContext(); | 
| Node* result_pair = | 
| __ CallRuntimeN(function_id, context, first_arg, args_count, 2); | 
| - | 
| // Store the results in <first_return> and <first_return + 1> | 
| Node* first_return_reg = __ BytecodeOperandReg(3); | 
| Node* second_return_reg = __ NextRegister(first_return_reg); |