Thanks Anton for furthur review. updated patch uploaded.
http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1410 src/arm/macro-assembler-arm.cc:1410: void MacroAssembler::EnterExitFramePrologue(int unwind_space) { On 2011/01/13 12:47:08, antonm wrote:
Thanks a lot for introducing this macros. But you still keep the code duplicated (see ::EnterExitFrame above)
EnterExitFrame satisfies the builtin (argc/argv) interface, registers are hard coded and does alignment. in our case, the interface is different and alignment needs to happen after pushing the additional space for v8::arguments and the alignment needs to be generic (as i intend to reuse this for load callback as well). given this i wasnt sure how to reuse this. let me know if you have some idea. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1412 src/arm/macro-assembler-arm.cc:1412: stm(db_w, sp, fp.bit() | ip.bit() | lr.bit()); On 2011/01/13 12:47:08, antonm wrote:
Please, retain the original comment which describes the semantics of
this push Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1426 src/arm/macro-assembler-arm.cc:1426: // Create space for the args On 2011/01/13 12:47:08, antonm wrote:
nit: period at the end of the comment, please
Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1433 src/arm/macro-assembler-arm.cc:1433: tst(sp, Operand(frame_alignment_mask)); On 2011/01/13 12:47:08, antonm wrote:
please, keep the comment
Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1471 src/arm/macro-assembler-arm.cc:1471: // patch the exit frame pc and Call the api function! On 2011/01/13 12:47:08, antonm wrote:
nit: [P]atch and [c]all
Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1481 src/arm/macro-assembler-arm.cc:1481: // otherwise set it to undefined On 2011/01/13 12:47:08, antonm wrote:
nit: period at the end of the comment.
Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1482 src/arm/macro-assembler-arm.cc:1482: LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq); On 2011/01/13 12:47:08, antonm wrote:
it looks like you forgot cmp(r0, Operand(0))
sorry, i realized that in my tests. fixed. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1501 src/arm/macro-assembler-arm.cc:1501: mov(ip, Operand(ExternalReference::the_hole_value_location())); On 2011/01/13 12:47:08, antonm wrote:
please, use LoadRoot.
Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h#newcode646 src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. On 2011/01/13 12:47:08, antonm wrote:
nit: we use |<parameter name>| convention. Ditto for unwind_space
below Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h#newcode646 src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. On 2011/01/13 12:47:08, antonm wrote:
maybe some other name? allocation_size or something like that?
hmm arg_stack_size is taken from x64. do you insist on changing it http://codereview.chromium.org/6170001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
