Generally, the approach looks sound, but there is a bunch of details that
need
to be ironed out.
http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc#newcode83
src/arm/assembler-arm.cc:83: unsigned standard_features =
static_cast<unsigned>(OS::CpuFeaturesImpliedByPlatform() |
80 col
http://codereview.chromium.org/10706002/diff/6005/src/arm/assembler-arm.cc#newcode749
src/arm/assembler-arm.cc:749: if (fits_shifter(-static_cast<int>(imm32),
rotate_imm, immed_8, NULL)) {
these seem to be unrelated changes, probably to make some version of GCC
work? Perhaps more these to a separate CL?
http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#newcode7522
src/arm/code-stubs-arm.cc:7522: __ Push(lr, r1, r0);
In general, you can't make guarantees about stack alignment within
generated code. We align on the way into C code in the CEntryStub, but
since you don't use that, you will actually have to handle both cases,
the aligned and unaligned, here if you expect the callee to have an
aligned stack.
http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#newcode7528
src/arm/code-stubs-arm.cc:7528: __ sub(r0, lr, Operand(0xC));
Define constants for this.
http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#newcode7531
src/arm/code-stubs-arm.cc:7531: __ add(r1, sp, Operand(0x0C));
Is it possible to use an existing constant in frames-arm.h for this?
http://codereview.chromium.org/10706002/diff/6005/src/arm/code-stubs-arm.cc#newcode7534
src/arm/code-stubs-arm.cc:7534:
reinterpret_cast<Address>(V8::GetFunctionEntryHookDispatcher()));
If you really need the dispatcher for ARM only, then it should just be
part of the ARM implementation and not exposed through the API. I am not
sure why you need the indirect dispatch, I think you can just use:
mov(pc, target_reg, LeaveCC, al);?
http://codereview.chromium.org/10706002/diff/6005/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/10706002/diff/6005/src/code-stubs.h#newcode1148
src/code-stubs.h:1148:
Two empty lines between classes
http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc#newcode7480
src/ia32/code-stubs-ia32.cc:7480: __ db(0x0F); // rdtsc();
Any reason to not add this to assembler-ia32.h?
http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc#newcode7491
src/ia32/code-stubs-ia32.cc:7491: __ lea(eax, Operand(esp, 0xC));
Constants? (maybe from frame-ia32.h, also applies to x64)
http://codereview.chromium.org/10706002/diff/6005/src/ia32/code-stubs-ia32.cc#newcode7496
src/ia32/code-stubs-ia32.cc:7496: __ sub(eax, Immediate(5));
constant?
http://codereview.chromium.org/10706002/diff/6005/src/v8.cc
File src/v8.cc (right):
http://codereview.chromium.org/10706002/diff/6005/src/v8.cc#newcode185
src/v8.cc:185: profiler_entry_hook(function, stack_pointer, entry_time);
80 col?
http://codereview.chromium.org/10706002/diff/6005/src/v8.h
File src/v8.h (right):
http://codereview.chromium.org/10706002/diff/6005/src/v8.h#newcode107
src/v8.h:107: static FunctionEntryHook GetFunctionEntryHookDispatcher();
Why does the dispatch need to be exposed in the API? I don't think this
is something clients need to be able to call. In any case, there seem to
be no tests of this.
http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):
http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc#newcode6425
src/x64/code-stubs-x64.cc:6425: __ db(0x0F); // rdtsc();
Add to the assembler?
http://codereview.chromium.org/10706002/diff/6005/src/x64/code-stubs-x64.cc#newcode6462
src/x64/code-stubs-x64.cc:6462: code->set_is_pregenerated(true);
Why do you need to pre-generate on x64 and ia32 but not on ARM?
http://codereview.chromium.org/10706002/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
