Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Revert of [Interpreter] Collect type feedback for 'new' in the byteco…
Browse files Browse the repository at this point in the history 
…de handler (patchset #6 id:100001 of https://codereview.chromium.org/2190293003/ )

Reason for revert:
[Sheriff] Fails on nosnap debug:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20debug/builds/8403

Original issue's description:
> [Interpreter] Collect type feedback for 'new' in the bytecode handler
>
> Collect type feedback in the bytecode handler for 'new' bytecode. The
> earlier cl (https://codereview.chromium.org/2153433002/) was reverted
> because that implementation did not collect allocation site feedback.
> This regressed delta blue by an order of magnitude. This implementation
> includes collection of allocation site feedback.
>
> BUG=v8:4280, v8:4780
> LOG=N
>
> Committed: https://crrev.com/9d5e6129c4c7f9cbfe81a5fad2a470f219fe137c
> Cr-Commit-Position: refs/heads/master@{#38364}

[email protected],[email protected],[email protected],[email protected]
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4280, v8:4780

Review-Url: https://codereview.chromium.org/2212343002
Cr-Commit-Position: refs/heads/master@{#38368}
  • Loading branch information
@mi-ac
mi-ac authored and Commit bot committed Aug 5, 2016
1 parent 2648162 commit dea16c9
Show file tree
Hide file tree
Showing 30 changed files with 141 additions and 543 deletions.
3 changes: 1 addition & 2 deletions src/arm/interface-descriptors-arm.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,7 @@ void InterpreterPushArgsAndConstructDescriptor::InitializePlatformSpecific(
r0, // argument count (not including receiver)
r3, // new target
r1, // constructor to call
r2, // allocation site feedback if available, undefined otherwise
r4 // address of the first argument
r2 // address of the first argument
};
data->InitializePlatformSpecific(arraysize(registers), registers);
}
Expand Down
3 changes: 1 addition & 2 deletions src/arm64/interface-descriptors-arm64.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,7 @@ void InterpreterPushArgsAndConstructDescriptor::InitializePlatformSpecific(
x0, // argument count (not including receiver)
x3, // new target
x1, // constructor to call
x2, // allocation site feedback if available, undefined otherwise
x4 // address of the first argument
x2 // address of the first argument
};
data->InitializePlatformSpecific(arraysize(registers), registers);
}
Expand Down
32 changes: 7 additions & 25 deletions src/builtins/arm/builtins-arm.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,6 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
__ mov(r3, Operand(r3, LSL, kPointerSizeLog2));
__ sub(r3, r2, r3);

// TODO(mythria): Add a stack check before pushing arguments.
// Push the arguments.
Generate_InterpreterPushArgs(masm, r2, r3, r4);

Expand All @@ -1207,44 +1206,27 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
}

// static
void Builtins::Generate_InterpreterPushArgsAndConstructImpl(
MacroAssembler* masm, CallableType construct_type) {
void Builtins::Generate_InterpreterPushArgsAndConstruct(MacroAssembler* masm) {
// ----------- S t a t e -------------
// -- r0 : argument count (not including receiver)
// -- r3 : new target
// -- r1 : constructor to call
// -- r2 : allocation site feedback if available, undefined otherwise.
// -- r4 : address of the first argument
// -- r2 : address of the first argument
// -----------------------------------

// Find the address of the last argument.
__ mov(r5, Operand(r0, LSL, kPointerSizeLog2));
__ sub(r5, r4, r5);
__ mov(r4, Operand(r0, LSL, kPointerSizeLog2));
__ sub(r4, r2, r4);

// Push a slot for the receiver to be constructed.
__ mov(ip, Operand::Zero());
__ push(ip);

// TODO(mythria): Add a stack check before pushing arguments.
// Push the arguments.
Generate_InterpreterPushArgs(masm, r4, r5, r6);
Generate_InterpreterPushArgs(masm, r2, r4, r5);

__ AssertUndefinedOrAllocationSite(r2, r5);
if (construct_type == CallableType::kJSFunction) {
__ AssertFunction(r1);

// Tail call to the function-specific construct stub (still in the caller
// context at this point).
__ ldr(r4, FieldMemOperand(r1, JSFunction::kSharedFunctionInfoOffset));
__ ldr(r4, FieldMemOperand(r4, SharedFunctionInfo::kConstructStubOffset));
// Jump to the construct function.
__ add(pc, r4, Operand(Code::kHeaderSize - kHeapObjectTag));

} else {
DCHECK_EQ(construct_type, CallableType::kAny);
// Call the constructor with r0, r1, and r3 unmodified.
__ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
}
// Call the constructor with r0, r1, and r3 unmodified.
__ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
}

void Builtins::Generate_InterpreterEnterBytecodeDispatch(MacroAssembler* masm) {
Expand Down
31 changes: 7 additions & 24 deletions src/builtins/arm64/builtins-arm64.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1186,7 +1186,6 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
__ lsl(x3, x3, kPointerSizeLog2);
__ sub(x4, x2, x3);

// TODO(mythria): Add a stack check before pushing arguments.
// Push the arguments.
Label loop_header, loop_check;
__ Mov(x5, jssp);
Expand Down Expand Up @@ -1214,14 +1213,12 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
}

// static
void Builtins::Generate_InterpreterPushArgsAndConstructImpl(
MacroAssembler* masm, CallableType construct_type) {
void Builtins::Generate_InterpreterPushArgsAndConstruct(MacroAssembler* masm) {
// ----------- S t a t e -------------
// -- x0 : argument count (not including receiver)
// -- x3 : new target
// -- x1 : constructor to call
// -- x2 : allocation site feedback if available, undefined otherwise
// -- x4 : address of the first argument
// -- x2 : address of the first argument
// -----------------------------------

// Find the address of the last argument.
Expand All @@ -1231,38 +1228,24 @@ void Builtins::Generate_InterpreterPushArgsAndConstructImpl(
// Set stack pointer and where to stop.
__ Mov(x6, jssp);
__ Claim(x5, 1);
__ sub(x7, x6, x5);
__ sub(x4, x6, x5);

// Push a slot for the receiver.
__ Str(xzr, MemOperand(x6, -kPointerSize, PreIndex));

Label loop_header, loop_check;
// TODO(mythria): Add a stack check before pushing arguments.
// Push the arguments.
__ B(&loop_check);
__ Bind(&loop_header);
// TODO(rmcilroy): Push two at a time once we ensure we keep stack aligned.
__ Ldr(x5, MemOperand(x4, -kPointerSize, PostIndex));
__ Ldr(x5, MemOperand(x2, -kPointerSize, PostIndex));
__ Str(x5, MemOperand(x6, -kPointerSize, PreIndex));
__ Bind(&loop_check);
__ Cmp(x6, x7);
__ Cmp(x6, x4);
__ B(gt, &loop_header);

__ AssertUndefinedOrAllocationSite(x2, x6);
if (construct_type == CallableType::kJSFunction) {
__ AssertFunction(x1);

// Tail call to the function-specific construct stub (still in the caller
// context at this point).
__ Ldr(x4, FieldMemOperand(x1, JSFunction::kSharedFunctionInfoOffset));
__ Ldr(x4, FieldMemOperand(x4, SharedFunctionInfo::kConstructStubOffset));
__ Add(x4, x4, Code::kHeaderSize - kHeapObjectTag);
__ Br(x4);
} else {
DCHECK_EQ(construct_type, CallableType::kAny);
// Call the constructor with x0, x1, and x3 unmodified.
__ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
}
// Call the constructor with x0, x1, and x3 unmodified.
__ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
}

void Builtins::Generate_InterpreterEnterBytecodeDispatch(MacroAssembler* masm) {
Expand Down
22 changes: 0 additions & 22 deletions src/builtins/builtins-interpreter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,27 +50,5 @@ void Builtins::Generate_InterpreterPushArgsAndTailCallFunction(
CallableType::kJSFunction);
}

Handle<Code> Builtins::InterpreterPushArgsAndConstruct(
CallableType function_type) {
switch (function_type) {
case CallableType::kJSFunction:
return InterpreterPushArgsAndConstructFunction();
case CallableType::kAny:
return InterpreterPushArgsAndConstruct();
}
UNREACHABLE();
return Handle<Code>::null();
}

void Builtins::Generate_InterpreterPushArgsAndConstruct(MacroAssembler* masm) {
return Generate_InterpreterPushArgsAndConstructImpl(masm, CallableType::kAny);
}

void Builtins::Generate_InterpreterPushArgsAndConstructFunction(
MacroAssembler* masm) {
return Generate_InterpreterPushArgsAndConstructImpl(
masm, CallableType::kJSFunction);
}

} // namespace internal
} // namespace v8
7 changes: 1 addition & 6 deletions src/builtins/builtins.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,9 @@ namespace internal {
ASM(InterpreterMarkBaselineOnReturn) \
ASM(InterpreterPushArgsAndCall) \
ASM(InterpreterPushArgsAndCallFunction) \
ASM(InterpreterPushArgsAndConstruct) \
ASM(InterpreterPushArgsAndTailCall) \
ASM(InterpreterPushArgsAndTailCallFunction) \
ASM(InterpreterPushArgsAndConstruct) \
ASM(InterpreterPushArgsAndConstructFunction) \
ASM(InterpreterEnterBytecodeDispatch) \
ASM(InterpreterOnStackReplacement) \
\
Expand Down Expand Up @@ -582,7 +581,6 @@ class Builtins {
Handle<Code> InterpreterPushArgsAndCall(
TailCallMode tail_call_mode,
CallableType function_type = CallableType::kAny);
Handle<Code> InterpreterPushArgsAndConstruct(CallableType function_type);

Code* builtin(Name name) {
// Code::cast cannot be used here since we access builtins
Expand Down Expand Up @@ -627,9 +625,6 @@ class Builtins {
MacroAssembler* masm, TailCallMode tail_call_mode,
CallableType function_type);

static void Generate_InterpreterPushArgsAndConstructImpl(
MacroAssembler* masm, CallableType function_type);

static void Generate_DatePrototype_GetField(MacroAssembler* masm,
int field_index);

Expand Down
131 changes: 27 additions & 104 deletions src/builtins/ia32/builtins-ia32.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -702,20 +702,19 @@ void Builtins::Generate_InterpreterMarkBaselineOnReturn(MacroAssembler* masm) {
}

static void Generate_InterpreterPushArgs(MacroAssembler* masm,
Register array_limit,
Register start_address) {
Register array_limit) {
// ----------- S t a t e -------------
// -- start_address : Pointer to the last argument in the args array.
// -- ebx : Pointer to the last argument in the args array.
// -- array_limit : Pointer to one before the first argument in the
// args array.
// -----------------------------------
Label loop_header, loop_check;
__ jmp(&loop_check);
__ bind(&loop_header);
__ Push(Operand(start_address, 0));
__ sub(start_address, Immediate(kPointerSize));
__ Push(Operand(ebx, 0));
__ sub(ebx, Immediate(kPointerSize));
__ bind(&loop_check);
__ cmp(start_address, array_limit);
__ cmp(ebx, array_limit);
__ j(greater, &loop_header, Label::kNear);
}

Expand All @@ -741,8 +740,7 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
__ neg(ecx);
__ add(ecx, ebx);

// TODO(mythria): Add a stack check before pushing the arguments.
Generate_InterpreterPushArgs(masm, ecx, ebx);
Generate_InterpreterPushArgs(masm, ecx);

// Call the target.
__ Push(edx); // Re-push return address.
Expand All @@ -760,115 +758,40 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
}

// static
void Builtins::Generate_InterpreterPushArgsAndConstructImpl(
MacroAssembler* masm, CallableType construct_type) {
void Builtins::Generate_InterpreterPushArgsAndConstruct(MacroAssembler* masm) {
// ----------- S t a t e -------------
// -- eax : the number of arguments (not including the receiver)
// -- edx : the new target
// -- edi : the constructor
// -- ebx : allocation site feedback (if available or undefined)
// -- ecx : the address of the first argument to be pushed. Subsequent
// -- ebx : the address of the first argument to be pushed. Subsequent
// arguments should be consecutive above this, in the same order as
// they are to be pushed onto the stack.
// -----------------------------------

// Store edi, edx onto the stack. We need two extra registers
// so store edi, edx temporarily on stack.
// Pop return address to allow tail-call after pushing arguments.
__ Pop(ecx);

// Push edi in the slot meant for receiver. We need an extra register
// so store edi temporarily on stack.
__ Push(edi);
__ Push(edx);

// We have to pop return address and the two temporary registers before we
// can push arguments onto the stack. we do not have any free registers so
// update the stack and copy them into the correct places on the stack.
// current stack =====> required stack layout
// | | | edx | (2) <-- esp(1)
// | | | edi | (3)
// | | | return addr | (4)
// | | | arg N | (5)
// | edx | <-- esp | .... |
// | edi | | arg 0 |
// | return addr | | receiver slot |

// First increment the stack pointer to the correct location.
// we need additional slots for arguments and the receiver.
// Step 1 - compute the required increment to the stack.
__ mov(edx, eax);
__ shl(edx, kPointerSizeLog2);
__ add(edx, Immediate(kPointerSize));

#ifdef _MSC_VER
// TODO(mythria): Move it to macro assembler.
// In windows, we cannot increment the stack size by more than one page
// (mimimum page size is 4KB) without accessing at least one byte on the
// page. Check this:
// https://msdn.microsoft.com/en-us/library/aa227153(v=vs.60).aspx.
const int page_size = 4 * 1024;
Label check_offset, update_stack_pointer;
__ bind(&check_offset);
__ cmp(edx, page_size);
__ j(less, &update_stack_pointer);
__ sub(esp, Immediate(page_size));
// Just to touch the page, before we increment further.
__ mov(Operand(esp, 0), Immediate(0));
__ sub(edx, Immediate(page_size));
__ jmp(&check_offset);
__ bind(&update_stack_pointer);
#endif

// TODO(mythria): Add a stack check before updating the stack pointer.

// Step 1 - Update the stack pointer.
__ sub(esp, edx);

// Step 2 move edx to the correct location. Move edx first otherwise
// we may overwrite when eax = 0 or 1, basically when the source and
// destination overlap. We at least need one extra slot for receiver,
// so no extra checks are required to avoid copy.
__ mov(edi, Operand(esp, eax, times_pointer_size, 1 * kPointerSize));
__ mov(Operand(esp, 0), edi);

// Step 3 move edi to the correct location
__ mov(edi, Operand(esp, eax, times_pointer_size, 2 * kPointerSize));
__ mov(Operand(esp, 1 * kPointerSize), edi);

// Step 4 move return address to the correct location
__ mov(edi, Operand(esp, eax, times_pointer_size, 3 * kPointerSize));
__ mov(Operand(esp, 2 * kPointerSize), edi);

// Step 5 copy arguments to correct locations.
__ mov(edx, eax);
// Find the address of the last argument.
__ mov(edi, eax);
__ neg(edi);
__ shl(edi, kPointerSizeLog2);
__ add(edi, ebx);

Label loop_header, loop_check;
__ jmp(&loop_check);
__ bind(&loop_header);
__ mov(edi, Operand(ecx, 0));
__ mov(Operand(esp, edx, times_pointer_size, 2 * kPointerSize), edi);
__ sub(ecx, Immediate(kPointerSize));
__ sub(edx, Immediate(1));
__ bind(&loop_check);
__ cmp(edx, Immediate(0));
__ j(greater, &loop_header, Label::kNear);
Generate_InterpreterPushArgs(masm, edi);

// Restore edi and edx.
__ Pop(edx);
__ Pop(edi);

__ AssertUndefinedOrAllocationSite(ebx);
if (construct_type == CallableType::kJSFunction) {
// Tail call to the function-specific construct stub (still in the caller
// context at this point).
__ AssertFunction(edi);

__ mov(ecx, FieldOperand(edi, JSFunction::kSharedFunctionInfoOffset));
__ mov(ecx, FieldOperand(ecx, SharedFunctionInfo::kConstructStubOffset));
__ lea(ecx, FieldOperand(ecx, Code::kHeaderSize));
__ jmp(ecx);
} else {
DCHECK_EQ(construct_type, CallableType::kAny);
// Restore the constructor from slot on stack. It was pushed at the slot
// meant for receiver.
__ mov(edi, Operand(esp, eax, times_pointer_size, 0));

// Call the constructor with unmodified eax, edi, edx values.
__ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
}
// Re-push return address.
__ Push(ecx);

// Call the constructor with unmodified eax, edi, ebi values.
__ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
}

void Builtins::Generate_InterpreterEnterBytecodeDispatch(MacroAssembler* masm) {
Expand Down
Loading

0 comments on commit dea16c9

Please sign in to comment.