Thanks Erik and Anton for your comments. Uploaded patch with comments
addressed.
Could you please take another look.
http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.cc#newcode2710
src/arm/code-stubs-arm.cc:2710: // popping the args.
On 2011/01/21 17:56:36, antonm wrote:
I think you should add a reference to EnterExitFrame, something like
"EnterExitFrame below will store ip as sp on exit"---it was obvious
when it was
in the same macro, but not now
Done.
http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.cc#newcode2723
src/arm/code-stubs-arm.cc:2723: __ mov(r5, Operand(r1));
On 2011/01/21 14:28:40, Erik Corry wrote:
I don't see the benefit of moving this outside the EnterExitFrame, but
if you
are going to do it then you should update the comment in
macro-assembler-arm.h.
Updated comment in macro-assembler-arm.h.
http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.h#newcode459
src/arm/code-stubs-arm.h:459: // Stub used when returning from direct
native call to generated code.
On 2011/01/21 17:56:36, antonm wrote:
English is not native to me, so feel free to ignore.
May you rework the comment? It's not obvious what's the difference
between the
code stub and the stub is.
Maybe something along these lines: "Trampoline stub to call into
native
code.\n\nTo call safely into native code in the presence of compacting
GC (which
can move code objects) we need to keep the code which called into
native pinned
in the memory. Currently the simplest approach is to generate such
stub early
enough so it can never be moved by GC".
My wording is far from perfect, just a stub :)
Thanks. I think your description is more accurate and complete.
Done.
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.cc#newcode547
src/arm/macro-assembler-arm.cc:547: int pending_pushes = stack_space +
4; // 4 pushes in this function.
On 2011/01/21 17:56:36, antonm wrote:
May you explain: it looks like only pending_pushes % 2 are used in
this function
which looks odd and this + 4 addition looks strange as well. What do
I miss?
sorry i miss your comment.
BTW, do we have tests for API function invocations with many (> 3)
arguments?
the call api requests space for 4 arguments (Arguments::implicit_args,
values, length, is_construct) and existing tests e.g.
(FastApiCallback_SimpleSignature) already cover this case.
Maybe i miss your comment.
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.h#newcode259
src/arm/macro-assembler-arm.h:259: // |stack_space| pending pushes, used
for alignment before call to C.
On 2011/01/21 17:56:36, antonm wrote:
Erik, it was my request. I saw this style in various parts of v8 and
assumed
it's the default. Please, tell us your ultimate word on it.
Zaheer, sorry for luring you into this.
On 2011/01/21 14:28:40, Erik Corry wrote:
> We haven't used this |parameter_name| style elsewhere as far as I
can see.
> Also, this comment has not been updated to reflect that the ip has
sp that
> should be restored. Also the comment is out of date because it
mentions r6 as
> being set up by the EnterExitFrame, whereas in fact it needs to be
set up by
the
> caller.
i have replaced |stack_space| -> stack_space -
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#newcode800
src/arm/simulator-arm.cc:800: void* external_function, bool fp_return,
ExternalReference::Type type) {
On 2011/01/21 14:28:40, Erik Corry wrote:
You shouldn't need both the bool for the fp_return and the external
reference
type. They can both be folded into the external reference type.
Done.
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#newcode1543
src/arm/simulator-arm.cc:1543: // This signature supports direct call in
to js function native callback
On 2011/01/21 17:56:36, antonm wrote:
I'd use API function instead of js function here
Done.
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#newcode1590
src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target =
On 2011/01/21 17:56:36, antonm wrote:
please, consider refactoring the common logic of logging and stack
alignment
checks
The log is unique to each case, the stack alignment check refactoring
also has a problem - moving it down will mean it will print after
executing function, moving it before will impact readability of log. Any
ideas.
http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc#newcode622
src/arm/stub-cache-arm.cc:622: __ stm(ib, sp, r5.bit() | r6.bit());
On 2011/01/21 17:56:36, antonm wrote:
Please, add a comment
Done.
http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc#newcode624
src/arm/stub-cache-arm.cc:624: // r2 points to calldata as expected by
Arguments class (refer layout above)
On 2011/01/21 17:56:36, antonm wrote:
nit: either call_data or call data, please
Done.
http://codereview.chromium.org/6170001/diff/90001/src/heap.h
File src/heap.h (right):
http://codereview.chromium.org/6170001/diff/90001/src/heap.h#newcode121
src/heap.h:121: #if V8_TARGET_ARCH_ARM
On 2011/01/21 17:56:36, antonm wrote:
Maybe we should restructure that differently:
I'd prefer to have a single STRONG_ROOT_LIST_ARM macro which could be
empty, or
keep DirectCEntryStub, or both.
The code could look like
#if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP
...
#elif V8_TARGET_ARCH_ARM
#else
#endif
What do you think?
Agree. i simplified one more bit and removed STRONG_ROOT_LIST_ARM.
#if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP
#define STRONG_ROOT_LIST(V)
\
UNCONDITIONAL_STRONG_ROOT_LIST(V)
\
V(Code, re_c_entry_code, RegExpCEntryCode)
\
V(Code, direct_c_entry_code, DirectCEntryCode)
#elif V8_TARGET_ARCH_ARM
#define STRONG_ROOT_LIST(V)
\
UNCONDITIONAL_STRONG_ROOT_LIST(V)
\
V(Code, direct_c_entry_code, DirectCEntryCode)
#else
#define STRONG_ROOT_LIST(V)
\
UNCONDITIONAL_STRONG_ROOT_LIST(V)
#endif
Is this ok?
http://codereview.chromium.org/6170001/diff/90001/src/top.h
File src/top.h (right):
http://codereview.chromium.org/6170001/diff/90001/src/top.h#newcode36
src/top.h:36: #ifdef USE_SIMULATOR
On 2011/01/21 17:56:36, antonm wrote:
why this change?
There's a circular reference with introduction of new type
ExternalReference::Type. it is declared in assembler.h needs to be used
in simulator.h (RedirectExternalReference), but assembler includes
simulator.h through top.h.
http://codereview.chromium.org/6170001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
