MIPS simple function calls

[Alexandre, 2010-03-15 22:03:31]

Hi,

Here is some update for MIPS to do a simple global function call.
It has been tested to execute some code like:

function foo() {
  return 0x1234;
}
foo();

The code was tested with a customized shell:
in shell.cc use
v8::Handle<v8::Context> context = v8::Context::New();
instead of
8::Handle<v8::Context> context = v8::Context::New(NULL, global);

Regards,

Alexandre

[Alexandre, 2010-03-15 22:04:27]

I'm correcting the forgotten lint errors...

On 2010/03/15 22:03:31, Alexandre wrote:
> Hi,
> 
> Here is some update for MIPS to do a simple global function call.
> It has been tested to execute some code like:
> 
> function foo() {
>   return 0x1234;
> }
> foo();
> 
> The code was tested with a customized shell:
> in shell.cc use
> v8::Handle<v8::Context> context = v8::Context::New();
> instead of
> 8::Handle<v8::Context> context = v8::Context::New(NULL, global);
> 
> Regards,
> 
> Alexandre

[Søren Thygesen Gjesse, 2010-03-17 14:49:09]

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

[Alexandre, 2010-03-18 17:16:08]

Thanks for the review,

I'll be working on branch delay slots. I think I will make the MacroAssembler
generate nop instructions. Using the branch delay slot would then need to use
directly the Assembler instead.
I will also add some tests for function calls. Rather than adding tests in
test-assembler-mips.cc for such 'high level' tests, shouldn't I add a
test-mips-codegen.cc file in which future tests could also be implemented?

Paul from MIPS has also fixed some bugs and implemented more tests. We will
upload the patch after this issue.

Regards,

Alexandre

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: 
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> 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
> // ...

Done.

http://codereview.chromium.org/1018001/diff/16001/17002#newcode87
src/mips/builtins-mips.cc:87: // * Stack:
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> All other comments holding a stack layout have TOS first.

Using TOS first from now on.

http://codereview.chromium.org/1018001/diff/16001/17002#newcode121
src/mips/builtins-mips.cc:121: __ lw(t0, MemOperand(s0));  // read next
parameter
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> 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).

Done.

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: 
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> See other comment regarding the format these state comments.
> 

Done.

http://codereview.chromium.org/1018001/diff/16001/17007#newcode246
src/mips/codegen-mips.cc:246: masm_->addiu(sp, sp, (scope()->num_parameters() +
1)*kPointerSize);
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> Spaces around binary operation.

Done.

http://codereview.chromium.org/1018001/diff/16001/17007#newcode1155
src/mips/codegen-mips.cc:1155: // check for failure result
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> Format comment with uppercase letter and period - more below.

Done.

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();
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> This code is not really used.

Indeed. I remove it.

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) {
On 2010/03/17 14:49:09, Søren Gjesse wrote:
> Please format the comment without *'s, e.g.
> 
> // State
> // a1: function
> // ra: return address

Done.

[Søren Thygesen Gjesse, 2010-03-19 10:11:23]

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.

[Alexandre, 2010-03-22 12:35:58]

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.

[Alexandre, 2010-03-22 13:59:25]

Hi,

I actually added some bugs while correcting the code. I'm checking it right now.

Alexandre

On 2010/03/22 12:35:58, Alexandre wrote:
> 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.

[Alexandre, 2010-03-22 16:46:16]

Hi,

I corrected the bugs and added a new test for function calls in
test/cctest/test-mips.cc . We can use this file to implement all further high
level MIPS tests.

Regards,

Alexandre

[Søren Thygesen Gjesse, 2010-03-23 11:15:40]

When I run tools/test.py --simulator=mips it turns out that the test
test-mips/MIPS_function_calls fails as it hits a large number of
UNIMPLEMENTED_MIPS. Is  Was that the intention?

Also two more comments:

1. Regarding writing below the stack pointer in
MacroAssembler::SetupAlignedCall. I don't know how MIPS Linux uses the stack
when handling interrupts, so it might not actually cause problems. However I
think that even if that is the case it should be avoided - even it we have to
generate a bit more code.

2. I think you are right that the code generator should not use the at register
and allow assembler/macro assembler to use it at will. on x64 we have reserved a
register as kScratchRegister for use by the code generator, but that might not
be relevant for you.

And finally for comitting, are you doing this work while for Sigma Designs Inc.?

http://codereview.chromium.org/1018001/diff/45001/46014
File test/cctest/test-mips.cc (right):

http://codereview.chromium.org/1018001/diff/45001/46014#newcode38
test/cctest/test-mips.cc:38: TEST(MIPS_function_calls) {
Please rename the test to MIPSFunctionCall.

http://codereview.chromium.org/1018001/diff/45001/46014#newcode42
test/cctest/test-mips.cc:42: const char* c_source = "function foo() { return
0x1234; }; foo();";
Can't you do without ::v8:: here?

[Alexandre, 2010-03-23 13:35:11]

Hi,

I corrected test-mips.cc and changed SetupAlignedCall code.

I think your problem running tools/test.py --simulator=mips is that your
src/flag-definitions.h is not good for MIPS.
You need these diffs to have it work:

Index: src/flag-definitions.h
===================================================================
--- src/flag-definitions.h	(revision 4138)
+++ src/flag-definitions.h	(working copy)
@@ -122,7 +122,7 @@
 // bootstrapper.cc
 DEFINE_string(expose_natives_as, NULL, "expose natives in global object")
 DEFINE_string(expose_debug_as, NULL, "expose debug in global object")
-DEFINE_string(natives_file, NULL, "alternative natives file")
+DEFINE_string(natives_file, "", "alternative natives file")
 DEFINE_bool(expose_gc, false, "expose gc extension")
 DEFINE_int(stack_trace_limit, 10, "number of stack frames to capture")
 
@@ -145,7 +145,7 @@
 DEFINE_bool(strict, false, "strict error checking")
 DEFINE_int(min_preparse_length, 1024,
            "minimum length for automatic enable preparsing")
-DEFINE_bool(full_compiler, true, "enable dedicated backend for run-once code")
+DEFINE_bool(full_compiler, false, "enable dedicated backend for run-once code")
 DEFINE_bool(fast_compiler, false, "enable speculative optimizing backend")
 DEFINE_bool(always_full_compiler, false,
             "try to use the dedicated run-once backend for all code")

Regards,

Alexandre

http://codereview.chromium.org/1018001/diff/45001/46014
File test/cctest/test-mips.cc (right):

http://codereview.chromium.org/1018001/diff/45001/46014#newcode38
test/cctest/test-mips.cc:38: TEST(MIPS_function_calls) {
On 2010/03/23 11:15:40, Søren Gjesse wrote:
> Please rename the test to MIPSFunctionCall.

Done.

http://codereview.chromium.org/1018001/diff/45001/46014#newcode42
test/cctest/test-mips.cc:42: const char* c_source = "function foo() { return
0x1234; }; foo();";
On 2010/03/23 11:15:40, Søren Gjesse wrote:
> Can't you do without ::v8:: here?

I can. Removed it.

[Søren Thygesen Gjesse, 2010-03-23 15:07:01]

On 2010/03/23 13:35:11, Alexandre wrote:
> Hi,
> 
> I corrected test-mips.cc and changed SetupAlignedCall code.
> 
> I think your problem running tools/test.py --simulator=mips is that your
> src/flag-definitions.h is not good for MIPS.
> You need these diffs to have it work:
> 
> Index: src/flag-definitions.h
> ===================================================================
> --- src/flag-definitions.h	(revision 4138)
> +++ src/flag-definitions.h	(working copy)
> @@ -122,7 +122,7 @@
>  // bootstrapper.cc
>  DEFINE_string(expose_natives_as, NULL, "expose natives in global object")
>  DEFINE_string(expose_debug_as, NULL, "expose debug in global object")
> -DEFINE_string(natives_file, NULL, "alternative natives file")
> +DEFINE_string(natives_file, "", "alternative natives file")
>  DEFINE_bool(expose_gc, false, "expose gc extension")
>  DEFINE_int(stack_trace_limit, 10, "number of stack frames to capture")
>  
> @@ -145,7 +145,7 @@
>  DEFINE_bool(strict, false, "strict error checking")
>  DEFINE_int(min_preparse_length, 1024,
>             "minimum length for automatic enable preparsing")
> -DEFINE_bool(full_compiler, true, "enable dedicated backend for run-once
code")
> +DEFINE_bool(full_compiler, false, "enable dedicated backend for run-once
code")
>  DEFINE_bool(fast_compiler, false, "enable speculative optimizing backend")
>  DEFINE_bool(always_full_compiler, false,
>              "try to use the dedicated run-once backend for all code")
> 
> Regards,
> 
> Alexandre
> 
> http://codereview.chromium.org/1018001/diff/45001/46014
> File test/cctest/test-mips.cc (right):
> 
> http://codereview.chromium.org/1018001/diff/45001/46014#newcode38
> test/cctest/test-mips.cc:38: TEST(MIPS_function_calls) {
> On 2010/03/23 11:15:40, Søren Gjesse wrote:
> > Please rename the test to MIPSFunctionCall.
> 
> Done.
> 
> http://codereview.chromium.org/1018001/diff/45001/46014#newcode42
> test/cctest/test-mips.cc:42: const char* c_source = "function foo() { return
> 0x1234; }; foo();";
> On 2010/03/23 11:15:40, Søren Gjesse wrote:
> > Can't you do without ::v8:: here?
> 
> I can. Removed it.

Landed through http://codereview.chromium.org/1140004. Closing this issue.