Updates and fixes for MIPS support.

Updates and fixes for MIPS support.

[Alexandre, 2010-03-25 10:40:44]

Hi,

Here is an update for MIPS code, done by Paul Lind from MIPS.
It contains fixes and support for floating point in the simulator as well as new
tests, and a few other fixes.

Regards,

Alexandre

[Søren Thygesen Gjesse, 2010-04-06 18:10:02]

LGTM

Thanks for the patch, I will land it when the minor comments have been addressed
and the patch synced with bleedng_edge HEAD.

http://codereview.chromium.org/1320006/diff/1/9
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/1320006/diff/1/9#newcode474
src/mips/assembler-mips.h:474: void mfc1(Register rt, FPURegister fs);
Will mthc1 and mfhc1 be implemented again - or are they not needed?

http://codereview.chromium.org/1320006/diff/1/4
File src/mips/codegen-mips.cc (right):

http://codereview.chromium.org/1320006/diff/1/4#newcode1184
src/mips/codegen-mips.cc:1184: __ bind(&retry);  // pass last failure (v0) as
parameter (a0) when retrying
Uppercase initial letter in comment and end with period.

http://codereview.chromium.org/1320006/diff/1/3
File test/cctest/test-mips.cc (right):

http://codereview.chromium.org/1320006/diff/1/3#newcode47
test/cctest/test-mips.cc:47: 
Why not leave the function without any arguments in the test as well (I mean
have both the previous and the new test)?

[Alexandre, 2010-04-08 09:16:11]

Hi Søren,

I just fixed the code.
I'll rebase it on bleeding edge once it is accepted.

I also have a question: is it possible to share the right to modify an issue
with somebody else? In our case I'd like Paul to be able to also answer and
upload some code for an issue I have created.

Thanks,

Alexandre

http://codereview.chromium.org/1320006/diff/1/9
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/1320006/diff/1/9#newcode474
src/mips/assembler-mips.h:474: void mfc1(Register rt, FPURegister fs);
On 2010/04/06 18:10:02, Søren Gjesse wrote:
> Will mthc1 and mfhc1 be implemented again - or are they not needed?

Answer from Paul Lind:
They are not needed.
The common MIPS ABI in use today uses the FPU regs in a
"compatibility mode", where we do not use the upper 32-bits of each 64-bit
FPU register (rather, we use pairs of 32-bit regs to hold doubles.)
Therefore, there is no need for these instructions at this time.

Here at mips, we're looking at future ABI changes that could changes this,
but it will not happen soon.

http://codereview.chromium.org/1320006/diff/1/4
File src/mips/codegen-mips.cc (right):

http://codereview.chromium.org/1320006/diff/1/4#newcode1184
src/mips/codegen-mips.cc:1184: __ bind(&retry);  // pass last failure (v0) as
parameter (a0) when retrying
On 2010/04/06 18:10:02, Søren Gjesse wrote:
> Uppercase initial letter in comment and end with period.

Done.

http://codereview.chromium.org/1320006/diff/1/3
File test/cctest/test-mips.cc (right):

http://codereview.chromium.org/1320006/diff/1/3#newcode47
test/cctest/test-mips.cc:47: 
On 2010/04/06 18:10:02, Søren Gjesse wrote:
> Why not leave the function without any arguments in the test as well (I mean
> have both the previous and the new test)?

Ok I'll change this.

[Søren Thygesen Gjesse, 2010-04-08 09:33:12]

LGTM

http://codereview.chromium.org/1320006/diff/15001/16002
File test/cctest/test-mips.cc (right):

http://codereview.chromium.org/1320006/diff/15001/16002#newcode58
test/cctest/test-mips.cc:58: CHECK_EQ(0x80,  script->Run()->Int32Value());
I was actually thinking of testing both functions, maybe like this:

  const char* c_source_1 = "function foo() { return 0x1234; }; foo();";
  Local<String> source_1 = ::v8::String::New(c_source_1);
  Local<Script> script_1 = ::v8::Script::Compile(source_1);
  CHECK_EQ(0x1234,  script_1->Run()->Int32Value());

And then the other one c_source_2, source_2, script_2 - or make two scopes.

[Alexandre, 2010-04-08 09:55:13]

Hi,

I just updated test-mips.cc with a cleaner test.

Thanks,

Alexandre

http://codereview.chromium.org/1320006/diff/15001/16002
File test/cctest/test-mips.cc (right):

http://codereview.chromium.org/1320006/diff/15001/16002#newcode58
test/cctest/test-mips.cc:58: CHECK_EQ(0x80,  script->Run()->Int32Value());
On 2010/04/08 09:33:12, Søren Gjesse wrote:
> I was actually thinking of testing both functions, maybe like this:
> 
>   const char* c_source_1 = "function foo() { return 0x1234; }; foo();";
>   Local<String> source_1 = ::v8::String::New(c_source_1);
>   Local<Script> script_1 = ::v8::Script::Compile(source_1);
>   CHECK_EQ(0x1234,  script_1->Run()->Int32Value());
> 
> And then the other one c_source_2, source_2, script_2 - or make two scopes.

Changed.

[Søren Thygesen Gjesse, 2010-04-08 10:39:44]

On 2010/04/08 09:55:13, Alexandre wrote:
> Hi,
> 
> I just updated test-mips.cc with a cleaner test.
> 
> Thanks,
> 
> Alexandre
> 
> http://codereview.chromium.org/1320006/diff/15001/16002
> File test/cctest/test-mips.cc (right):
> 
> http://codereview.chromium.org/1320006/diff/15001/16002#newcode58
> test/cctest/test-mips.cc:58: CHECK_EQ(0x80,  script->Run()->Int32Value());
> On 2010/04/08 09:33:12, Søren Gjesse wrote:
> > I was actually thinking of testing both functions, maybe like this:
> > 
> >   const char* c_source_1 = "function foo() { return 0x1234; }; foo();";
> >   Local<String> source_1 = ::v8::String::New(c_source_1);
> >   Local<Script> script_1 = ::v8::Script::Compile(source_1);
> >   CHECK_EQ(0x1234,  script_1->Run()->Int32Value());
> > 
> > And then the other one c_source_2, source_2, script_2 - or make two scopes.
> 
> Changed.

Thanks, the patch is ready for commit, but the MIPS port needs to be updated to
build on current trunk. Feel free to do that in this patch if you like, or
create a separate one for that.

[Alexandre, 2010-05-20 16:59:37]

Hi Søren,

I finally took some time to update the MIPS support.

This update contains some fixes, more support for MIPS instructions, and support
for floating point with or without FPU.

If this looks ok I'll rebase it on bleeding edge.

Regards,

Alexandre

On 2010/04/08 10:39:44, Søren Gjesse wrote:
> On 2010/04/08 09:55:13, Alexandre wrote:
> > Hi,
> > 
> > I just updated test-mips.cc with a cleaner test.
> > 
> > Thanks,
> > 
> > Alexandre
> > 
> > http://codereview.chromium.org/1320006/diff/15001/16002
> > File test/cctest/test-mips.cc (right):
> > 
> > http://codereview.chromium.org/1320006/diff/15001/16002#newcode58
> > test/cctest/test-mips.cc:58: CHECK_EQ(0x80,  script->Run()->Int32Value());
> > On 2010/04/08 09:33:12, Søren Gjesse wrote:
> > > I was actually thinking of testing both functions, maybe like this:
> > > 
> > >   const char* c_source_1 = "function foo() { return 0x1234; }; foo();";
> > >   Local<String> source_1 = ::v8::String::New(c_source_1);
> > >   Local<Script> script_1 = ::v8::Script::Compile(source_1);
> > >   CHECK_EQ(0x1234,  script_1->Run()->Int32Value());
> > > 
> > > And then the other one c_source_2, source_2, script_2 - or make two
scopes.
> > 
> > Changed.
> 
> Thanks, the patch is ready for commit, but the MIPS port needs to be updated
to
> build on current trunk. Feel free to do that in this patch if you like, or
> create a separate one for that.

[Søren Thygesen Gjesse, 2010-05-25 08:52:57]

On 2010/05/20 16:59:37, Alexandre wrote:
> Hi Søren,
> 
> I finally took some time to update the MIPS support.
> 
> This update contains some fixes, more support for MIPS instructions, and
support
> for floating point with or without FPU.
> 
> If this looks ok I'll rebase it on bleeding edge.
> 
> Regards,
> 
> Alexandre
> 
> On 2010/04/08 10:39:44, Søren Gjesse wrote:
> > On 2010/04/08 09:55:13, Alexandre wrote:
> > > Hi,
> > > 
> > > I just updated test-mips.cc with a cleaner test.
> > > 
> > > Thanks,
> > > 
> > > Alexandre
> > > 
> > > http://codereview.chromium.org/1320006/diff/15001/16002
> > > File test/cctest/test-mips.cc (right):
> > > 
> > > http://codereview.chromium.org/1320006/diff/15001/16002#newcode58
> > > test/cctest/test-mips.cc:58: CHECK_EQ(0x80,  script->Run()->Int32Value());
> > > On 2010/04/08 09:33:12, Søren Gjesse wrote:
> > > > I was actually thinking of testing both functions, maybe like this:
> > > > 
> > > >   const char* c_source_1 = "function foo() { return 0x1234; }; foo();";
> > > >   Local<String> source_1 = ::v8::String::New(c_source_1);
> > > >   Local<Script> script_1 = ::v8::Script::Compile(source_1);
> > > >   CHECK_EQ(0x1234,  script_1->Run()->Int32Value());
> > > > 
> > > > And then the other one c_source_2, source_2, script_2 - or make two
> scopes.
> > > 
> > > Changed.
> > 
> > Thanks, the patch is ready for commit, but the MIPS port needs to be updated
> to
> > build on current trunk. Feel free to do that in this patch if you like, or
> > create a separate one for that.

The changes looks good to me with the comments addressed.

Are there any particular reason for switching the order of the Condition and
Label arguments to the Branch functions?

Please create 3 separate patch sets (gcl upload) for addressing the comments,
rebase to bleeding_edge (most likely will not compile), and making it compile
again.

[Søren Thygesen Gjesse, 2010-05-25 09:00:54]

And here are the comments.

http://codereview.chromium.org/1320006/diff/35001/36005
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/1320006/diff/35001/36005#newcode56
src/mips/assembler-mips.cc:56: supported_ |= 1u << FPU;
Only 2 space indent.

http://codereview.chromium.org/1320006/diff/35001/36005#newcode975
src/mips/assembler-mips.cc:975: // load to two 32-bit loads. This really should
be done in macro-assembler,
I think you should just remove "This really should be done in macro-assembler,
but this should be temporary...."

http://codereview.chromium.org/1320006/diff/35001/36005#newcode981
src/mips/assembler-mips.cc:981: // GenInstrImmediate(LDC1, src.rm(), fd,
src.offset_);
Please remove code in comments.

http://codereview.chromium.org/1320006/diff/35001/36005#newcode992
src/mips/assembler-mips.cc:992: // store to two 32-bit stores. This really
should be done in macro-assembler,
Ditto.

http://codereview.chromium.org/1320006/diff/35001/36005#newcode998
src/mips/assembler-mips.cc:998: // GenInstrImmediate(SDC1, src.rm(), fd,
src.offset_);
Ditto.

http://codereview.chromium.org/1320006/diff/35001/36008
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/1320006/diff/35001/36008#newcode142
src/mips/assembler-mips.h:142: void setcode(int f) {
setcode -> set_code

http://codereview.chromium.org/1320006/diff/35001/36003
File src/mips/codegen-mips.cc (right):

http://codereview.chromium.org/1320006/diff/35001/36003#newcode1285
src/mips/codegen-mips.cc:1285: __ addiu(sp, sp,
-StandardFrameConstants::kCArgsSlotsSize);
Please add a comment on the usage of the branch delay slot and on the
instruction where execution continues after return.

http://codereview.chromium.org/1320006/diff/35001/36006
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/1320006/diff/35001/36006#newcode597
src/mips/constants-mips.h:597: static const int kBArgsSlotsSize = 0 *
Instruction::kInstructionSize;
DO you need a separate constant for builtins? Aren't they always called like
JavaScript?

http://codereview.chromium.org/1320006/diff/35001/36022
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/1320006/diff/35001/36022#newcode125
src/mips/frames-mips.h:125: // C/C++ argument slots size.
Aren't these constants duplicated here and in constants-mips.h?

http://codereview.chromium.org/1320006/diff/35001/36004
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/1320006/diff/35001/36004#newcode55
src/mips/macro-assembler-mips.cc:55: bool ProtectBranchDelaySlot) { \
ProtectBranchDelaySlot -> protect_branch_delay_slot

http://codereview.chromium.org/1320006/diff/35001/36004#newcode142
src/mips/macro-assembler-mips.cc:142: Branch(2, NegateCondition(cond), src1,
src2);
Assert that lw generates 2 instructions? Or use a label?

http://codereview.chromium.org/1320006/diff/35001/36004#newcode525
src/mips/macro-assembler-mips.cc:525: if (ProtectBranchDelaySlot)
Either use {}'s or place the nop() on the same line as the if.

http://codereview.chromium.org/1320006/diff/35001/36004#newcode559
src/mips/macro-assembler-mips.cc:559: // instruction, as the location will be
remember for patching the target.
remember -> remembered

http://codereview.chromium.org/1320006/diff/35001/36004#newcode860
src/mips/macro-assembler-mips.cc:860: Branch(2, NegateCondition(cond), rs, rt);
Please use use either labels to jump or assert the number of instructions
generated in some way. Several places below.

http://codereview.chromium.org/1320006/diff/35001/36004#newcode1214
src/mips/macro-assembler-mips.cc:1214: addiu(sp, sp,
-StandardFrameConstants::kBArgsSlotsSize);
Please add a comment that the second addiu will be executed after return.

http://codereview.chromium.org/1320006/diff/35001/36004#newcode1224
src/mips/macro-assembler-mips.cc:1224: addiu(sp, sp,
-StandardFrameConstants::kBArgsSlotsSize);
Ditto.

http://codereview.chromium.org/1320006/diff/35001/36020
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/1320006/diff/35001/36020#newcode46
src/mips/macro-assembler-mips.h:46: // Unless we know exactly what we do.
Therefore we create another scratch reg.
Remove "Unless we know exactly what we do."?

http://codereview.chromium.org/1320006/diff/35001/36020#newcode65
src/mips/macro-assembler-mips.h:65: // Arguments macros
Are you sure that using these macros here are beneficial?

http://codereview.chromium.org/1320006/diff/35001/36020#newcode245
src/mips/macro-assembler-mips.h:245: Branch(3, cond, tst1, Operand(tst2));
The constant 3 relies on Addu only generates one instruction. Could that be
asserted?

http://codereview.chromium.org/1320006/diff/35001/36020#newcode329
src/mips/macro-assembler-mips.h:329: // Clobber t0, t1, t2.
Clobber -> Clobbers