Re-establish mips basic infrastructure.

Re-establish mips basic infrastructure.

This commit adds current working versions of assembler, macro-assembler,
disassembler, and simulator.

All other mips arch files are replaced with stubbed-out versions that
will build.

Committed: http://code.google.com/p/v8/source/detail?r=7388
Arch independent files are updated as needed to support building and
running mips.

The only test is cctest/test-assembler-mips, and this passes on the
simulator and on mips hardware.

BUG=
TEST=

[Søren Thygesen Gjesse, 2011-03-21 16:05:19]

First round of comments, mainly for the platform independent files.

As you have probably seen the isolates branch was merged with bleeding_edge on
Friday (r7268 reverted in r7269 and finally landed in r7271. This means that
this patch will have to be updated to current bleeding_edge with isolates to be
committed. Looking at the changes in the ARM specific files in r7271 this should
be straight forward. You can run test.py with option --isolates to test with
several active isolates.

http://codereview.chromium.org/6709022/diff/1/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/6709022/diff/1/src/assembler.h#newcode361
src/assembler.h:361: // Code and Embedded Object pointers in mips are stored
split across
I don't like these platform dependent members here - if this is currently
unavoidable they should at least be inside #ifdef/#endif.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:607: // ASSERT(L->pos() == kEndOfChain ||
L->is_linked());
Code in comments.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:624: if (rmode == RelocInfo::NONE) {
Use

return rmode != RelocInfo::NONE

instead of the if statement.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:988: 
You should probably consider getting rid of ppep-hole optimizations going
forward. For crankshaft we have disbled it as it was to difficult to keep
control of the code generated when jumping from optimized code to un-optimized
code.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:1767: } else {
Instead of UNIMPLEMENTED_MIPS() this should probable be UNREACHABLE(). Or even
better just put in ASSERT(mips32r2).

More of these below.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:2072: // if ( ! (((instr1 & kOpcodeMask) == LUI &&
(instr2 & kOpcodeMask) == ORI) ||
Code in comments.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc
File src/mips/cpu-mips.cc (right):

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode44
src/mips/cpu-mips.cc:44: #include "simulator.h"  // for cache flushing.
Please start comment with uppercase letter.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode53
src/mips/cpu-mips.cc:53: if (true || !CpuFeatures::IsSupported(FPU) ||
Serializer::enabled()) {
Please remove the true here. Disabling crankshaft for MIPS should be handled by
an #ifdef/#endif in flag-definitions.h.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode70
src/mips/cpu-mips.cc:70: #else  // simulator mode
#else comment should match the #if

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode77
src/mips/cpu-mips.cc:77: #endif    // #ifdef __mips
#endif comment should match the #if

http://codereview.chromium.org/6709022/diff/1/src/mips/disasm-mips.cc
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/6709022/diff/1/src/mips/disasm-mips.cc#newcode66
src/mips/disasm-mips.cc:66: #ifdef _MIPS_ARCH_MIPS32R2
Please consider moving this to constants-mips.h. See constants-arm.h for how
CAN_USE_ARMV7_INSTRUCTIONS is defined.

http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1195
src/objects-inl.h:1195: #else  // V8_TARGET_ARCH_MIPS
I think this should be put into the macro READ_DOUBLE_FIELD, and then there
should be two different macro definitions

#ifndef V8_TARGET_ARCH_MIPS
#define READ_DOUBLE_FIELD(p, offset) \
  <current definition>
#else
#define READ_DOUBLE_FIELD(p, offset) \
  <MIPS specific definition>
#endif

http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1212
src/objects-inl.h:1212: #else  // V8_TARGET_ARCH_MIPS
Ditto.

[Paul Lind, 2011-03-23 01:55:43]

Thanks for the code review! I have addressed the comments, and rebased the code
on bleeding_edge r7312.

http://codereview.chromium.org/6709022/diff/1/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/6709022/diff/1/src/assembler.h#newcode361
src/assembler.h:361: // Code and Embedded Object pointers in mips are stored
split across
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> I don't like these platform dependent members here - if this is currently
> unavoidable they should at least be inside #ifdef/#endif.
I have removed this for now, since relocation is not needed for the
assembler/sim tests. I have commented out the access to these members from
assembler-mips-inl.h.

Going forward, I will submit a single commit with our approach to relocation,
and maybe you can suggest a cleaner way than this to handle it.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:607: // ASSERT(L->pos() == kEndOfChain ||
L->is_linked());
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Code in comments.
Removed.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:624: if (rmode == RelocInfo::NONE) {
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Use
> 
> return rmode != RelocInfo::NONE
> 
> instead of the if statement.

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:988: 
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> You should probably consider getting rid of ppep-hole optimizations going
> forward. For crankshaft we have disbled it as it was to difficult to keep
> control of the code generated when jumping from optimized code to un-optimized
> code.

I've filed an issue to remove this. I see that it is still present in Arm, and
disabled for crankshaft. We will follow this for now, and remove it later.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:1767: } else {
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Instead of UNIMPLEMENTED_MIPS() this should probable be UNREACHABLE(). Or even
> better just put in ASSERT(mips32r2).
> 
> More of these below.

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newc...
src/mips/assembler-mips.cc:2072: // if ( ! (((instr1 & kOpcodeMask) == LUI &&
(instr2 & kOpcodeMask) == ORI) ||
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Code in comments.

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc
File src/mips/cpu-mips.cc (right):

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode44
src/mips/cpu-mips.cc:44: #include "simulator.h"  // for cache flushing.
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Please start comment with uppercase letter.

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode53
src/mips/cpu-mips.cc:53: if (true || !CpuFeatures::IsSupported(FPU) ||
Serializer::enabled()) {
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Please remove the true here. Disabling crankshaft for MIPS should be handled
by
> an #ifdef/#endif in flag-definitions.h.

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode70
src/mips/cpu-mips.cc:70: #else  // simulator mode
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> #else comment should match the #if

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode77
src/mips/cpu-mips.cc:77: #endif    // #ifdef __mips
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> #endif comment should match the #if

Done.

http://codereview.chromium.org/6709022/diff/1/src/mips/disasm-mips.cc
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/6709022/diff/1/src/mips/disasm-mips.cc#newcode66
src/mips/disasm-mips.cc:66: #ifdef _MIPS_ARCH_MIPS32R2
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Please consider moving this to constants-mips.h. See constants-arm.h for how
> CAN_USE_ARMV7_INSTRUCTIONS is defined.

Done.

http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1195
src/objects-inl.h:1195: #else  // V8_TARGET_ARCH_MIPS
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> I think this should be put into the macro READ_DOUBLE_FIELD, and then there
> should be two different macro definitions
> 
> #ifndef V8_TARGET_ARCH_MIPS
> #define READ_DOUBLE_FIELD(p, offset) \
>   <current definition>
> #else
> #define READ_DOUBLE_FIELD(p, offset) \
>   <MIPS specific definition>
> #endif

The code has been moved to the READ_DOUBLE_FIELD macro as you suggest.

However, I saw no way to leave this as a pure macro, as the macro needs to
evaluate to an expression of type double that can be returned by HeapNumber::
value(). All approaches to multiline macros that I know (for instance, wrapping
in do { ... } while(0)), end up as a compound statement, not an expression.

Therefore I used a static inline function, and had the macro call that. I am
open to better suggestions.

http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1212
src/objects-inl.h:1212: #else  // V8_TARGET_ARCH_MIPS
On 2011/03/21 16:05:19, Søren Gjesse wrote:
> Ditto.

This one could have been implemented as pure macro, but I chose to implement
this with static inline, as was done above, for consistency.

Maybe the inline function names should me made more unique, possibly
mips_write_double_field()?

[Søren Thygesen Gjesse, 2011-03-23 23:38:42]

Next round of comments. I still have to review macro-assembler-mips and
simulator-mips.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:147: // Commenting out, to simplify
arch-independednt changes.
I suggest adding a TODO(mips) to the beginning of this comment for reference.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:164: // Commenting out, to simplify
arch-independednt changes.
Ditto.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:199: // The pc_ offset of 0 assumes mips patched
return sequence per
patched return sequence -> patched return sequence or patched debug break slot.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:208: // The pc_ offset of 0 assumes mips patched
return sequence per
Ditto.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:235: static const int kMinimalBufferSize = 4*KB;
Spaces around binary operations.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:409: bool ret = (opcode == SLL &&
Indention off - indent to parentesis.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:438: // We actually create a new lw instruction based
on the original one.
Format this

Instr temp_instr = LW |
                      (instr & kRsFieldMask) |
                      (instr & kRtFieldMask) |
                      (offset & kImm16Mask);

as

Instr temp_instr =
    LW | (instr & kRsFieldMask) | (instr & kRtFieldMask) | (offset &
kImm16Mask);

if it fits in the 80 chars or

Instr temp_instr = LW | (instr & kRsFieldMask) | (instr & kRtFieldMask)
    | (offset & kImm16Mask);

if not.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:544: ASSERT(0 <= pos && pos <= pc_offset());  // must
have a valid binding position
Case + full stop in comment.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:884: BlockTrampolinePoolFor(1);
This additional blocking for one instruction is that for the delay slot? If a
BlockTrampolinePoolScope always blocks for one instruction after the scope maybe
that should be part of the BlockTrampolinePoolScope. I see this same pattern
below.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1242: ASSERT(rd.is_valid() && rt.is_valid() &&
is_uint5(sa));
Please change pattern

if (mips32r2) {
  ... something ...
} else {
  ASSERT(mips32r2)
}

to


ASSERT(mips32r2)
... something ...

It is used a number of times below.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1272: } else {  // Offset > 16 bits, use multiple
instructions to load.
Consider refactoring

ASSERT(!rs.rm().is(at));
lui(at, rs.offset_ >> 16);
ori(at, at, rs.offset_ & 0xffff);
addu(at, at, rs.rm());

into a helper function.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1274: lui(at, rs.offset_ >> 16);
Constants for 16 and 0xffff?

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1567: rt.code_ = (cc & 0x0003)<<2 | 1;
Spaces around << (more below).

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1588: // Ins instr has 'rt' field as dest, and two
uint5: msb, lsb
End comment with full stop.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:172: // Currently f0 is used as a scratch register.
This comment about f2 does not seems to be what the code below actually does.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:237: const FPURegister f0 = { 0 };  // return value in
hard float mode
Start comment with uppercase and end with full stop.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:249: const FPURegister f12 = { 12 };  // arg in hard
float mode
arg -> arg 0? Start comment with uppercase and end with full stop.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:251: const FPURegister f14 = { 14 };  // arg in hard
float mode
arg -> arg 1? Start comment with uppercase and end with full stop.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:486: // These values are used in serialization process
and must be zero for
in -> in the

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:998: next_label_ = start + slot_count*2*kInstrSize;
Spaces between binary operations (more below).

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:1002: int bound() {
If this function is just an accessor for next_slot_ shouldn't it be called
next_slot instead of bound?

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:1039: static const int kTrampolineInst =
Only 4 space indent of line starting with "2 *".

http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc#newc...
src/mips/disasm-mips.cc:211: out_buffer_pos_ += OS::SNPrintF(out_buffer_ +
out_buffer_pos_,
Indentation. More below. Some of these fit one line.

http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc#newc...
src/mips/disasm-mips.cc:513: }
End case with {}'s like this:

  break;
}

instead of

  }
  break;

http://codereview.chromium.org/6709022/diff/5001/src/mips/frames-mips.h
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/frames-mips.h#newco...
src/mips/frames-mips.h:109: // TODO(regis): Use a patched sp value on the stack
instead.
regis -> mips

http://codereview.chromium.org/6709022/diff/5001/test/cctest/test-assembler-m...
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/test/cctest/test-assembler-m...
test/cctest/test-assembler-mips.cc:331: #ifdef DEBUG
Do you want to have this printing?

[Søren Thygesen Gjesse, 2011-03-24 22:11:19]

Done with all files.

When the comments has been addressed this should be committable.

Regarding the preprocessor symbol mips32r2 I have a few comments. Should this be
a CPU feature like FPU with CPU feature check and CPU feature scope (on ARM
ARMv7 is a CPU feature)? If not why not have it as a global boolean instead of
0/1? You can save that to a separate change.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (left):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#o...
src/mips/assembler-mips.cc:478: ASSERT(fd.is_valid() && fs.is_valid() &&
ft.is_valid());
Shouldn't this and all other FPU instructions start with:

ASSERT(CpuFeatures::IsEnabled(FPU));

?

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2162: void
MacroAssembler::AllocateHeapNumber(Register result,
Indentation off.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2171: AllocateInNewSpace(HeapNumber::kSize,
Indentation off.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2183: #ifdef DEBUG
Code in comments.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2472: lw(result,
Indentation.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2573: lw(scratch1, FieldMemOperand(object,
HeapNumber::kExponentOffset));
What is the purpose of this block?

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2955: #endif
The comment about vstrm is ARM specific.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:3040: return 2 * kPointerSize;  //
TODO(REBASE): use above flog & remove this line.
REBASE -> mips, flog -> flag

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:45: // The programmer should know that the
MacroAssembler may clobber these two,
two -> three

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:105: void Name(bool ProtectBranchDelaySlot =
true); \
ProtectBranchDelaySlot -> protect_branch_delay_slot (quite a few places)

How about using an enum instead of a bool.

enum BranchDelaySlot {
  PROTECT_BRANCH_DELAY_SLOT,
  EXPOSE_BRANCH_DELAY_SLOT,
}

Seeing EXPOSE_BRANCH_DELAY_SLOT as an argument instead of just false seems more
clear.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:227: Condition cc,  // eq for new space, ne
otherwise
End comment with full stop.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:273: // ie. check if is is a sll zero_reg,
zero_reg, <type> (referenced as
is is -> it is

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:282: static inline int GetCodeMarker(Instr
instr) {
Isn't there functions in assembler to extract opcode, rt, rs and sa from an
instructions?

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:293: int type = (sllzz &&
Doesn't this fit one line? If not please format like this

int type =
    <the rest>

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:448: Addu(sp, sp, Operand(2 * -kPointerSize));
I would prefer -(2 * kPointerSize) instead of 2 * -kPointerSize. What is the
reason for not having Subu?

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:472: 
Why do we have both push and Push? (same for pop)

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:570: void AlignStack(int offset);    //
TODO(REBASE) : remove this function.
REBASE -> mips

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:759: //  sw(t0, MemOperand(sp,
MacroAssembler::kCFuncArg_5));
How about adding CFunctionArgumentOperand (like ContextOperand) and use

sw(t0, CFunctionArgumentOperand(5))

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:309: PrintF("\n\n");
Only 2 space indent.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:1368: // RS field is equal to 00001
Is there any reason to check for R2 in the simulator? It should be the code
generation that should only generate R1 instructions if R2 is not enabled.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:2103: alu_out |= rt & mask;
Format

  }
  break;

as

  break;
}

More below.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:2303: PrintF("  0x%08x  %s\n",
reinterpret_cast<intptr_t>(instr),
Indentation.

[Paul Lind, 2011-03-26 18:39:17]

Responded to all comments. Thanks for the review.

The new patchset is uploaded, but I have not rebased on the tip yet. Will do
that later this weekend.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:147: // Commenting out, to simplify
arch-independednt changes.
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> I suggest adding a TODO(mips) to the beginning of this comment for reference.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:164: // Commenting out, to simplify
arch-independednt changes.
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:199: // The pc_ offset of 0 assumes mips patched
return sequence per
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> patched return sequence -> patched return sequence or patched debug break
slot.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl....
src/mips/assembler-mips-inl.h:208: // The pc_ offset of 0 assumes mips patched
return sequence per
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (left):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#o...
src/mips/assembler-mips.cc:478: ASSERT(fd.is_valid() && fs.is_valid() &&
ft.is_valid());
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Shouldn't this and all other FPU instructions start with:
> 
> ASSERT(CpuFeatures::IsEnabled(FPU));
> 
> ?

Yes. Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:235: static const int kMinimalBufferSize = 4*KB;
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Spaces around binary operations.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:409: bool ret = (opcode == SLL &&
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Indention off - indent to parentesis.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:438: // We actually create a new lw instruction based
on the original one.
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Format this
> 
> Instr temp_instr = LW |
>                       (instr & kRsFieldMask) |
>                       (instr & kRtFieldMask) |
>                       (offset & kImm16Mask);
> 
> as
> 
> Instr temp_instr =
>     LW | (instr & kRsFieldMask) | (instr & kRtFieldMask) | (offset &
> kImm16Mask);
> 
> if it fits in the 80 chars or
> 
> Instr temp_instr = LW | (instr & kRsFieldMask) | (instr & kRtFieldMask)
>     | (offset & kImm16Mask);
> 
> if not.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:544: ASSERT(0 <= pos && pos <= pc_offset());  // must
have a valid binding position
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Case + full stop in comment.

Fixed this one, and all the others in the file. Sorry about these silly
formatting errors. I know the style guide, at least for these simple cases.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:884: BlockTrampolinePoolFor(1);
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> This additional blocking for one instruction is that for the delay slot? If a
> BlockTrampolinePoolScope always blocks for one instruction after the scope
maybe
> that should be part of the BlockTrampolinePoolScope. I see this same pattern
> below.

Yes, it is for the delay slot.

In this file, that is the pattern, but it is not the pattern elsewhere. In our
current code (not this stubbed-out version), we have 27 instances of
BlockTrampolinePoolScope, and 15 of those have this pattern, 11 here, 4 in
codegen-mips.cc.

While it wouldn't hurt to block pool-emission everywhere for the one extra
instruction, it doesn't seem called for to me. I guess I prefer the explicit
call when it is needed.

If you feel otherwise, I will put it in, let me know.

I did comment each instance in this file, to mention the delay slot.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1242: ASSERT(rd.is_valid() && rt.is_valid() &&
is_uint5(sa));
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Please change pattern
> 
> if (mips32r2) {
>   ... something ...
> } else {
>   ASSERT(mips32r2)
> }
> 
> to
> 
> 
> ASSERT(mips32r2)
> ... something ...
> 
> It is used a number of times below.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1272: } else {  // Offset > 16 bits, use multiple
instructions to load.
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Consider refactoring
> 
> ASSERT(!rs.rm().is(at));
> lui(at, rs.offset_ >> 16);
> ori(at, at, rs.offset_ & 0xffff);
> addu(at, at, rs.rm());
> 
> into a helper function.

Added helper function LoadRegPlusOffsetToAt()

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1274: lui(at, rs.offset_ >> 16);
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Constants for 16 and 0xffff?

Added (and corrected existing) constants in constants-mips.h, and corrected this
same usage pattern to use those constants in several other places, including
macro-assembler-mips

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1567: rt.code_ = (cc & 0x0003)<<2 | 1;
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Spaces around << (more below).

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#n...
src/mips/assembler-mips.cc:1588: // Ins instr has 'rt' field as dest, and two
uint5: msb, lsb
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> End comment with full stop.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:172: // Currently f0 is used as a scratch register.
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> This comment about f2 does not seems to be what the code below actually does.
Removed bogus comments.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:237: const FPURegister f0 = { 0 };  // return value in
hard float mode
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Start comment with uppercase and end with full stop.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:249: const FPURegister f12 = { 12 };  // arg in hard
float mode
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> arg -> arg 0? Start comment with uppercase and end with full stop.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:251: const FPURegister f14 = { 14 };  // arg in hard
float mode
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> arg -> arg 1? Start comment with uppercase and end with full stop.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:486: // These values are used in serialization process
and must be zero for
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> in -> in the

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:998: next_label_ = start + slot_count*2*kInstrSize;
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Spaces between binary operations (more below).

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:1002: int bound() {
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> If this function is just an accessor for next_slot_ shouldn't it be called
> next_slot instead of bound?

This accessor was unused. I deleted it.

http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#ne...
src/mips/assembler-mips.h:1039: static const int kTrampolineInst =
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Only 4 space indent of line starting with "2 *".

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc#newc...
src/mips/disasm-mips.cc:211: out_buffer_pos_ += OS::SNPrintF(out_buffer_ +
out_buffer_pos_,
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Indentation. More below. Some of these fit one line.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc#newc...
src/mips/disasm-mips.cc:513: }
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> End case with {}'s like this:
> 
>   break;
> }
> 
> instead of
> 
>   }
>   break;

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/frames-mips.h
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/frames-mips.h#newco...
src/mips/frames-mips.h:109: // TODO(regis): Use a patched sp value on the stack
instead.
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> regis -> mips

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2162: void
MacroAssembler::AllocateHeapNumber(Register result,
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Indentation off.

Done here, and in several nearby places.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2171: AllocateInNewSpace(HeapNumber::kSize,
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Indentation off.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2183: #ifdef DEBUG
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Code in comments.

Removed. Was an early artifact of trying to align HeapNumbers to 8-byte boundary
for mips, which we might want to revisit someday.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2472: lw(result,
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2573: lw(scratch1, FieldMemOperand(object,
HeapNumber::kExponentOffset));
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> What is the purpose of this block?

There is no reason for the extra block-scope. Removed.

I also tightened up the code for mips. Older version copied the arm code a bit
too closely, trying to emulate an instruction we don't have.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:2955: #endif
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> The comment about vstrm is ARM specific.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.cc:3040: return 2 * kPointerSize;  //
TODO(REBASE): use above flog & remove this line.
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> REBASE -> mips, flog -> flag
Deleted that line, and started using FLAG_sim_Stack_alignment.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:45: // The programmer should know that the
MacroAssembler may clobber these two,
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> two -> three

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:105: void Name(bool ProtectBranchDelaySlot =
true); \
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> ProtectBranchDelaySlot -> protect_branch_delay_slot (quite a few places)
> 
> How about using an enum instead of a bool.
> 
> enum BranchDelaySlot {
>   PROTECT_BRANCH_DELAY_SLOT,
>   EXPOSE_BRANCH_DELAY_SLOT,
> }
> 
> Seeing EXPOSE_BRANCH_DELAY_SLOT as an argument instead of just false seems
more
> clear.

Yes, exactly! I did this yesterday before seeing your comment, have wanted to do
this one for some time. Names are slightly different, but the code is nicely
self-documenting now. The 'false' was always an annoyance :-)

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:227: Condition cc,  // eq for new space, ne
otherwise
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> End comment with full stop.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:273: // ie. check if is is a sll zero_reg,
zero_reg, <type> (referenced as
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> is is -> it is

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:282: static inline int GetCodeMarker(Instr
instr) {
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Isn't there functions in assembler to extract opcode, rt, rs and sa from an
> instructions?

There is not currently such a function. Routines are doing this manually using
the defined constants. I created an issue for this, but am not fixing it in this
pass. If you need it fixed immediately, please let me know.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:293: int type = (sllzz &&
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Doesn't this fit one line? If not please format like this
> 
> int type =
>     <the rest>

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:448: Addu(sp, sp, Operand(2 * -kPointerSize));
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> I would prefer -(2 * kPointerSize) instead of 2 * -kPointerSize. What is the
> reason for not having Subu?

I changed it to use Subu

Long story, mips arch has a subu 3-register instruction, analogous to addu.
However the arch does not have a subiu, subtract 16-bit immediate from register,
since the programmer or compiler can just use addiu with negative immediate
operand.

We have a Subu macro-instruction, which in its "immediate-operand" version will
use addiu and invert the value.

This code looks cleaner with the Subu, though some of us just forget to use that
cause we have already been brain-damaged.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:472: 
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Why do we have both push and Push? (same for pop)

It is historical.

There is a push() and pop() in arm, implemented in assembler using str/ldr. I'm
sure the original mips push/pop were modeled from that.

The Push() and Pop() came from the multi-register version in arm, where you can
push/pop in any order.

Currently, both of these are in use, on both mips and arm. I don't see a strong
reason to keep the lower case ones.

 I did not remove them in this change. Let me know if you want that.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:570: void AlignStack(int offset);    //
TODO(REBASE) : remove this function.
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> REBASE -> mips

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mip...
src/mips/macro-assembler-mips.h:759: //  sw(t0, MemOperand(sp,
MacroAssembler::kCFuncArg_5));
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> How about adding CFunctionArgumentOperand (like ContextOperand) and use
> 
> sw(t0, CFunctionArgumentOperand(5))

Cool idea!! Implemented. Thanks.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:309: PrintF("\n\n");
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Only 2 space indent.

Done.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:1368: // RS field is equal to 00001
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Is there any reason to check for R2 in the simulator? It should be the code
> generation that should only generate R1 instructions if R2 is not enabled.

I guess I could see this either way. But sure, it is not necessary. Removed all
the mip32r2 checks from sim.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:2103: alu_out |= rt & mask;
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Format
> 
>   }
>   break;
> 
> as
> 
>   break;
> }
> 
> More below.

Fixed, several spots.

http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#n...
src/mips/simulator-mips.cc:2303: PrintF("  0x%08x  %s\n",
reinterpret_cast<intptr_t>(instr),
On 2011/03/24 22:11:19, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/6709022/diff/5001/test/cctest/test-assembler-m...
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/6709022/diff/5001/test/cctest/test-assembler-m...
test/cctest/test-assembler-mips.cc:331: #ifdef DEBUG
On 2011/03/23 23:38:42, Søren Gjesse wrote:
> Do you want to have this printing?

Not useful now that things are working. Removed from all tests.

[Søren Thygesen Gjesse, 2011-03-28 13:06:28]

LGTM!

I am fine with the postponing some changes to a separate change.

Committed: http://code.google.com/p/v8/source/detail?r=7388