Here are some initial comments.
This is a rather big change, so I will need a couple of days.
Would it be possible to ass a simple test to test-mips.cc which just
compiles
and calls a function?
v8::HandleScope scope;
LocalContext env; // from cctest.h
const char* c_source = "function foo() { return 0x1234; }; foo();";
Local<String> source = String::New(c_source);
Local<Script> script = Script::Compile(source);
CHECK_EQ(0x1234, script->Run()->Int32Value());
http://codereview.chromium.org/1018001/diff/16001/17002
File src/mips/builtins-mips.cc (right):
http://codereview.chromium.org/1018001/diff/16001/17002#newcode78
src/mips/builtins-mips.cc:78:
Please format these comments without *'s, also there is no need to have
_'s in register content description.
// Registers
// a0: entry address
// a1: function
// ..
//
// Stack
// ...
http://codereview.chromium.org/1018001/diff/16001/17002#newcode87
src/mips/builtins-mips.cc:87: // * Stack:
All other comments holding a stack layout have TOS first.
http://codereview.chromium.org/1018001/diff/16001/17002#newcode117
src/mips/builtins-mips.cc:117: __ b(&entry);
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.
http://codereview.chromium.org/1018001/diff/16001/17002#newcode121
src/mips/builtins-mips.cc:121: __ lw(t0, MemOperand(s0)); // read next
parameter
read next parameter -> Read next parameter. Also for two more comments
below.
(I know we have end of line comments which does not have uppercase and
period, but we are trying to avoid these).
http://codereview.chromium.org/1018001/diff/16001/17002#newcode126
src/mips/builtins-mips.cc:126: __ Branch(ne, &loop, s0, Operand(t2));
Maybe just having Branch always emit nop might make thinsg easier and
less error prone.
http://codereview.chromium.org/1018001/diff/16001/17002#newcode168
src/mips/builtins-mips.cc:168: __ InvokeFunction(a1, actual,
CALL_FUNCTION);
Again having to remember to put in nop - especially after macro
assembler functions seems fragile.
http://codereview.chromium.org/1018001/diff/16001/17007
File src/mips/codegen-mips.cc (right):
http://codereview.chromium.org/1018001/diff/16001/17007#newcode129
src/mips/codegen-mips.cc:129:
See other comment regarding the format these state comments.
http://codereview.chromium.org/1018001/diff/16001/17007#newcode246
src/mips/codegen-mips.cc:246: masm_->addiu(sp, sp,
(scope()->num_parameters() + 1)*kPointerSize);
Spaces around binary operation.
http://codereview.chromium.org/1018001/diff/16001/17007#newcode1155
src/mips/codegen-mips.cc:1155: // check for failure result
Format comment with uppercase letter and period - more below.
http://codereview.chromium.org/1018001/diff/16001/17005
File src/mips/ic-mips.cc (right):
http://codereview.chromium.org/1018001/diff/16001/17005#newcode76
src/mips/ic-mips.cc:76: UNIMPLEMENTED_MIPS();
This code is not really used.
http://codereview.chromium.org/1018001/diff/16001/17011
File src/mips/stub-cache-mips.cc (right):
http://codereview.chromium.org/1018001/diff/16001/17011#newcode162
src/mips/stub-cache-mips.cc:162: Object*
StubCompiler::CompileLazyCompile(Code::Flags flags) {
Please format the comment without *'s, e.g.
// State
// a1: function
// ra: return address
http://codereview.chromium.org/1018001
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
