Hi, I just uploaded the changes. I still need to add a test.
Alexandre http://codereview.chromium.org/1018001/diff/16001/17002 File src/mips/builtins-mips.cc (right): http://codereview.chromium.org/1018001/diff/16001/17002#newcode117 src/mips/builtins-mips.cc:117: __ b(&entry); On 2010/03/17 14:49:09, Søren Gjesse wrote:
Please make a comment when placing an instruction in the branch delay
slot.
Maybe you should just always have nop there for now to make the code
more
readable (especially for people not used to this).
Of cause having the macro assembler handle it would be even better.
For an
unconditional jump like here that might not be that difficult.
Branching instructions and pseudo instructions were modified . MacroAssembler Branch and Jump always generate a nop in the branch delay slot; Assembler instructions will NOT generate a nop. Comments are added when the branch delay slot is used. http://codereview.chromium.org/1018001/diff/16001/17002#newcode126 src/mips/builtins-mips.cc:126: __ Branch(ne, &loop, s0, Operand(t2)); On 2010/03/17 14:49:09, Søren Gjesse wrote:
Maybe just having Branch always emit nop might make thinsg easier and
less error
prone.
See previous comment about the branch (pseudo)instructions. http://codereview.chromium.org/1018001/diff/16001/17002#newcode168 src/mips/builtins-mips.cc:168: __ InvokeFunction(a1, actual, CALL_FUNCTION); On 2010/03/17 14:49:09, Søren Gjesse wrote:
Again having to remember to put in nop - especially after macro
assembler
functions seems fragile.
See previous comments about branch instructions. 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. On 2010/03/19 10:11:23, Søren Gjesse wrote:
Maybe consider adding LoadExternalReference to macro assembler.
Added LoadExternalReference to MacroAssembler and added it where needed. http://codereview.chromium.org/1018001/diff/16001/17007#newcode1294 src/mips/codegen-mips.cc:1294: __ lw(s0, MemOperand(sp, (kNumCalleeSaved + 1)*kPointerSize + On 2010/03/19 10:11:23, Søren Gjesse wrote:
Spaces around binary operations.
Done. http://codereview.chromium.org/1018001/diff/16001/17007#newcode1323 src/mips/codegen-mips.cc:1323: __ li(t0, Operand(ExternalReference(Top::k_pending_exception_address))); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Remove two spaces before comment.
Done. 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) { On 2010/03/19 10:11:23, Søren Gjesse wrote:
Do you need a scratch register here, or is it possible to just use at?
Well we will indeed not need 'at' here. I didn't like using at somewhere else than in Assembler or MacroAssembler because it can be used without the user seeing it. It is less dangerous now that the Assembler instructions won't modify at. I change the code here with a big warning about this. http://codereview.chromium.org/1018001/diff/16001/17009#newcode872 src/mips/macro-assembler-mips.cc:872: // arguments' 8-byte alignment. On 2010/03/19 10:11:23, Søren Gjesse wrote:
Is it safe to write below the stack pointer? Even if it is I think we
should
avoid it.
Indentation.
I also don't like to write under sp, but as we want to save sp itself on the stack, this is either this or some bigger code... Indentation corrected. http://codereview.chromium.org/1018001/diff/16001/17009#newcode874 src/mips/macro-assembler-mips.cc:874: sw(sp, MemOperand(sp, -8)); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Comment that this is args + receiver, and that all args are word
sized.
2 -> kPointerSizeLog2
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode879 src/mips/macro-assembler-mips.cc:879: } On 2010/03/19 10:11:23, Søren Gjesse wrote:
Indentation + branch delay slot comment.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode969 src/mips/macro-assembler-mips.cc:969: InvokePrologue(expected, actual, Handle<Code>::null(), code, On 2010/03/19 10:11:23, Søren Gjesse wrote:
Indention.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode990 src/mips/macro-assembler-mips.cc:990: InvokePrologue(expected, actual, code, no_reg, On 2010/03/19 10:11:23, Søren Gjesse wrote:
Indention.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1007 src/mips/macro-assembler-mips.cc:1007: ASSERT(function.is(a1)); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Maybe add Register function = a1 as well.
The function register is already an argument. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1029 src/mips/macro-assembler-mips.cc:1029: void MacroAssembler::GetObjectType(Register function, On 2010/03/19 10:11:23, Søren Gjesse wrote:
Indention.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1043 src/mips/macro-assembler-mips.cc:1043: jalr(t9); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Comment when code in the branch delay slot.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1052 src/mips/macro-assembler-mips.cc:1052: jalr(target); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Comment when code in the branch delay slot.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1064 src/mips/macro-assembler-mips.cc:1064: jr(t9); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Comment when code in the branch delay slot.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1072 src/mips/macro-assembler-mips.cc:1072: jr(t9); On 2010/03/19 10:11:23, Søren Gjesse wrote:
Comment when code in the branch delay slot.
Done. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1244 src/mips/macro-assembler-mips.cc:1244: mov(fp, sp); // setup new frame pointer On 2010/03/19 10:11:23, Søren Gjesse wrote:
Please use initial uppercase letter and period for end of line
comments as well.
Several other places.
Done here and in other places found. http://codereview.chromium.org/1018001/diff/16001/17009#newcode1288 src/mips/macro-assembler-mips.cc:1288: void MacroAssembler::AlignStack(int offset) { On 2010/03/19 10:11:23, Søren Gjesse wrote:
The meaning 0(8) and 4(8) in the comment seems unclear.
I meant "modulo". This function has been patched and will be changed in the next patch. 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. On 2010/03/19 10:11:23, Søren Gjesse wrote:
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.
I added Register arguments here. http://codereview.chromium.org/1018001 -- v8-dev mailing list v8-dev@googlegroups.com 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.
