I have now bee through all the code, and in general to looks good. When you
have
addressed the comments I will have another look and try to build it. Please
do
not add more to this patch except maybe some simple tests.
I have not reviewed the pieces which are almost identical to the ARM port
that
throughly.
And also if at all possible it will be much easier if you can send smaller
patches. I know that this is all needed to get a call through to JavaScript
and
back, but in the future try to break patches into smaller pieces.
http://codereview.chromium.org/1018001/diff/16001/17007
File src/mips/codegen-mips.cc (right):
http://codereview.chromium.org/1018001/diff/16001/17007#newcode1183
src/mips/codegen-mips.cc:1183: // Retrieve the pending exception and
clear the variable.
Maybe consider adding LoadExternalReference to macro assembler.
http://codereview.chromium.org/1018001/diff/16001/17007#newcode1294
src/mips/codegen-mips.cc:1294: __ lw(s0, MemOperand(sp, (kNumCalleeSaved
+ 1)*kPointerSize +
Spaces around binary operations.
http://codereview.chromium.org/1018001/diff/16001/17007#newcode1323
src/mips/codegen-mips.cc:1323: __ li(t0,
Operand(ExternalReference(Top::k_pending_exception_address)));
Remove two spaces before comment.
http://codereview.chromium.org/1018001/diff/16001/17009
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/1018001/diff/16001/17009#newcode869
src/mips/macro-assembler-mips.cc:869: void
MacroAssembler::SetupAlignedCall(Register scratch, int arg_count) {
Do you need a scratch register here, or is it possible to just use at?
http://codereview.chromium.org/1018001/diff/16001/17009#newcode872
src/mips/macro-assembler-mips.cc:872: // arguments' 8-byte alignment.
Is it safe to write below the stack pointer? Even if it is I think we
should avoid it.
Indentation.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode874
src/mips/macro-assembler-mips.cc:874: sw(sp, MemOperand(sp, -8));
Comment that this is args + receiver, and that all args are word sized.
2 -> kPointerSizeLog2
http://codereview.chromium.org/1018001/diff/16001/17009#newcode879
src/mips/macro-assembler-mips.cc:879: }
Indentation + branch delay slot comment.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode969
src/mips/macro-assembler-mips.cc:969: InvokePrologue(expected, actual,
Handle<Code>::null(), code,
Indention.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode990
src/mips/macro-assembler-mips.cc:990: InvokePrologue(expected, actual,
code, no_reg,
Indention.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1007
src/mips/macro-assembler-mips.cc:1007: ASSERT(function.is(a1));
Maybe add Register function = a1 as well.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1029
src/mips/macro-assembler-mips.cc:1029: void
MacroAssembler::GetObjectType(Register function,
Indention.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1043
src/mips/macro-assembler-mips.cc:1043: jalr(t9);
Comment when code in the branch delay slot.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1052
src/mips/macro-assembler-mips.cc:1052: jalr(target);
Comment when code in the branch delay slot.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1064
src/mips/macro-assembler-mips.cc:1064: jr(t9);
Comment when code in the branch delay slot.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1072
src/mips/macro-assembler-mips.cc:1072: jr(t9);
Comment when code in the branch delay slot.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1244
src/mips/macro-assembler-mips.cc:1244: mov(fp, sp); // setup new frame
pointer
Please use initial uppercase letter and period for end of line comments
as well. Several other places.
http://codereview.chromium.org/1018001/diff/16001/17009#newcode1288
src/mips/macro-assembler-mips.cc:1288: void
MacroAssembler::AlignStack(int offset) {
The meaning 0(8) and 4(8) in the comment seems unclear.
http://codereview.chromium.org/1018001/diff/16001/17010
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/1018001/diff/16001/17010#newcode221
src/mips/macro-assembler-mips.h:221: // the builtin function to call in
register a1.
Please extend comment to include the callee saved registers which are
"live" after this. As far as I can see a2 is set to argv by this, and
used by calling code. Maybe add Register arguments for the registers set
by this and used afterwards, e.g.
EnterExitFrame(ExitFrame::Mode mode, Register setup_argv_in_this)
(not the best argument name though).
There might be other functions in the macro assembler which setups
callee saved registers used by the code which follows where the same
pattern can be used to document this.
http://codereview.chromium.org/1018001
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply
to this email with the words "REMOVE ME" as the subject.
