Added support for MIPS in architecture independent files....

Added support for MIPS in architecture independent files.
Code for MIPS assembler, disassembler, simulator, and debugger.
The simulator does not yet support all generated instructions and still needs some more tests.

[Søren Thygesen Gjesse, 2010-01-22 20:22:32]

I tried to compile the library using GCC 4.2 using (with the suggested changes
to SConstruct)

  $ scons simulator=mips

and there are a number of compiler warnings in src/mips/simulator-mips.cc.

After short-cutting these warnings the library can be built.

The sample shell and the developer shell can also be built using

  $ scons simulator=mips sample=shell
  $ scons simulator=mips d8

However there are some issues building the cctests using

  $ scons simulator=mips cctests

g++ -o obj/test/release/test-regexp.o -c -Wall -Werror -W -Wno-unused-parameter
-Wnon-virtual-dtor -pedantic -fno-strict-aliasing -O3 

-fomit-frame-pointer -fdata-sections -ffunction-sections -ansi -m32 -fno-rtti
-fno-exceptions -fvisibility=hidden -DV8_TARGET_ARCH_MIPS 

-DENABLE_DEBUGGER_SUPPORT -DENABLE_LOGGING_AND_PROFILING -Isrc -Isrc
test/cctest/test-regexp.cc
test/cctest/test-regexp.cc: In function 'void TestMacroAssembler()':
test/cctest/test-regexp.cc:1247: error: reference to 'byte' is ambiguous
src/globals.h:81: error: candidates are: typedef uint8_t v8::internal::byte
src/mips/constants-mips.h:115: error:                 typedef unsigned char
assembler::mips::byte
test/cctest/test-regexp.cc:1247: error: reference to 'byte' is ambiguous
src/globals.h:81: error: candidates are: typedef uint8_t v8::internal::byte
src/mips/constants-mips.h:115: error:                 typedef unsigned char
assembler::mips::byte
test/cctest/test-regexp.cc:1247: error: expected `;' before 'codes'
test/cctest/test-regexp.cc:1248: error: reference to 'byte' is ambiguous
src/globals.h:81: error: candidates are: typedef uint8_t v8::internal::byte
src/mips/constants-mips.h:115: error:                 typedef unsigned char
assembler::mips::byte
test/cctest/test-regexp.cc:1248: error: template argument 1 is invalid
test/cctest/test-regexp.cc:1248: error: 'codes' was not declared in this scope
scons: *** [obj/test/release/test-regexp.o] Error 1
scons: building terminated because of errors.

It is related to compiling test/cctest/test-regexp.cc. If I remove that from
test/cctest/SConscript

Please add test/cctest/test-assembler-mips.cc to test/cctests/SConscript

Index: test/cctest/SConscript
===================================================================
--- test/cctest/SConscript	(revision 3220)
+++ test/cctest/SConscript	(working copy)
@@ -52,7 +52,6 @@
     'test-log.cc',
     'test-log-utils.cc',
     'test-mark-compact.cc',
-    'test-regexp.cc',
     'test-serialize.cc',
     'test-sockets.cc',
     'test-spaces.cc',
@@ -71,6 +70,7 @@
   'arch:x64': ['test-assembler-x64.cc',
                'test-macro-assembler-x64.cc',
                'test-log-stack-tracer.cc'],
+  'arch:mips': ['test-assembler-mips.cc'],
   'os:linux':  ['test-platform-linux.cc'],
   'os:macos':  ['test-platform-macos.cc'],
   'os:nullos': ['test-platform-nullos.cc'],

You can then run

  $ tools/test.py --simulator=mips cctest/test-assembler-mips

and have two tests which are passing, however

  $ tools/test.py --simulator=mips

looks pretty scary :-), but it is possible to control which tests to run. If you
can try to add the following to the bottom of test/cctest/cctest.status 

[ $simulator == mips ]
test-api: SKIP
test-threads: SKIP
test-serialize: SKIP
test-log: SKIP
test-heap-profiler: SKIP
test-debug: SKIP
test-alloc: SKIP

Then running

  $ tools/test.py --simulator=mips cctest

Will not hang.

You should also mode the src/mips/test-* to test/cctests and turn the tests in
them into real tests. This will make it simpler to run the tests using the
tools/test.py script.

http://codereview.chromium.org/543161/diff/1/62
File SConstruct (right):

http://codereview.chromium.org/543161/diff/1/62#newcode864
SConstruct:864: return overrides
If you add the following here, then regexp=interpreted is not required to build
using SCons.

  if options['arch'] == 'mips':
    if ('regexp' in ARGUMENTS) and options['regexp'] == 'native':
      # Print a warning if native regexp is specified for mips
      print "Warning: forcing regexp to interpreted for mips"
    options['regexp'] = 'interpreted'

http://codereview.chromium.org/543161/diff/1/14
File src/SConscript (right):

http://codereview.chromium.org/543161/diff/1/14#newcode139
src/SConscript:139: mips/regexp-macro-assembler-mips.cc
You don't need mips/regexp-macro-assembler-mips.cc file when only supporting
interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/59
File src/jsregexp.cc (right):

http://codereview.chromium.org/543161/diff/1/59#newcode4478
src/jsregexp.cc:4478: #elif V8_TARGET_ARCH_MIPS
This change is not required when only supporting interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/27
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/1/27#newcode116
src/mips/assembler-mips-inl.h:116: return Assembler::target_address_at(pc_);
Remove two spaces before return.

http://codereview.chromium.org/543161/diff/1/27#newcode141
src/mips/assembler-mips-inl.h:141: Assembler::target_address_at(pc_)));
Four space indent when splitting line. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....

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

http://codereview.chromium.org/543161/diff/1/47#newcode178
src/mips/assembler-mips.cc:178: // WII? cf assembler.h 233 :Modes affected by
apply. Depends on arch.
I don't understand this comment.

http://codereview.chromium.org/543161/diff/1/47#newcode334
src/mips/assembler-mips.cc:334: 
We always try to have excatly two blank lines between functions.

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

http://codereview.chromium.org/543161/diff/1/54#newcode223
src/mips/assembler-mips.h:223: // immediate
Please start comment with an uppercase letter and end with a period.

http://codereview.chromium.org/543161/diff/1/54#newcode233
src/mips/assembler-mips.h:233: // reg
Ditto.

http://codereview.chromium.org/543161/diff/1/54#newcode251
src/mips/assembler-mips.h:251: // Class MemOperand represents a memory operand
in load and store instructions
Ditto.

http://codereview.chromium.org/543161/diff/1/54#newcode368
src/mips/assembler-mips.h:368: //  void beql(Register rs, Register rt, int16_t
offset);
Code in comment.

For now I suggest you add TODO(mips) comments like this:

  // TODO(mips) implement beql(Register rs, Register rt, int16_t offset);

Normally we try to only have TODO comments in the form of TODO(bugnumber) with a
tracking issue, but for this work in progress the general TODO(mips) will be
fine.

http://codereview.chromium.org/543161/diff/1/54#newcode386
src/mips/assembler-mips.h:386: // IMPORTANT NOTE: Never use the int16_t b(l)cond
version with a branch offset
Please indent comment to fit the source indention.

http://codereview.chromium.org/543161/diff/1/54#newcode391
src/mips/assembler-mips.h:391: const Operand& rt = Operand(zero_reg), Register
scratch = at);
Please remove one space.

http://codereview.chromium.org/543161/diff/1/54#newcode393
src/mips/assembler-mips.h:393: const Operand& rt = Operand(zero_reg), Register
scratch = at);
Ditto.

http://codereview.chromium.org/543161/diff/1/54#newcode410
src/mips/assembler-mips.h:410: //  INLINE(void jalr  (Register rs);)
Code in comments.

http://codereview.chromium.org/543161/diff/1/54#newcode548
src/mips/assembler-mips.h:548: void nop()  { sll(zero_reg, zero_reg, 0); }
Only one space between () and {

http://codereview.chromium.org/543161/diff/1/54#newcode777
src/mips/assembler-mips.h:777: #endif  // V8_ARM_ASSEMBLER_ARM_H_
V8_ARM_ASSEMBLER_ARM_H_ -> V8_MIPS_ASSEMBLER_MIPS_H_

http://codereview.chromium.org/543161/diff/1/40
File src/mips/builtins-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/40#newcode93
src/mips/builtins-mips.cc:93: // static void
EnterArgumentsAdaptorFrame(MacroAssembler* masm) {
Code in comments.

http://codereview.chromium.org/543161/diff/1/33
File src/mips/frames-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/33#newcode82
src/mips/frames-mips.cc:82: UNIMPLEMENTED_();
Code in comments.

http://codereview.chromium.org/543161/diff/1/33#newcode107
src/mips/frames-mips.cc:107: //  const int arguments =
Smi::cast(GetExpression(0))->value();
Ditto.

http://codereview.chromium.org/543161/diff/1/33#newcode116
src/mips/frames-mips.cc:116: // Internal frames have no arguments. The stack
pointer of the
Ditto.

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

http://codereview.chromium.org/543161/diff/1/46#newcode59
src/mips/macro-assembler-mips.cc:59: UNIMPLEMENTED();
Maybe use the UNIMPLEMENTED_MIPS macro.

http://codereview.chromium.org/543161/diff/1/46#newcode61
src/mips/macro-assembler-mips.cc:61: // TO_UPGRADE: Use a JAL instead of JALR if
the target is in the pc region and
Change TO_UPGRADE to TODO(mips).

http://codereview.chromium.org/543161/diff/1/35
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/1/35#newcode55
src/mips/macro-assembler-mips.h:55: class MacroAssembler: public Assembler {
I suggest you totally remove most of the functions of the macro assembler, and
add them as needed. There might be some of the ones you have copied that you
don't need and you might add some which does not make sense for other
architectures.

http://codereview.chromium.org/543161/diff/1/44
File src/mips/regexp-macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/44#newcode1
src/mips/regexp-macro-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
You don't need this file when only supporting interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/31
File src/mips/regexp-macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/1/31#newcode1
src/mips/regexp-macro-assembler-mips.h:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
You don't need this file when only supporting interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/52
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/52#newcode142
src/mips/simulator-mips.cc:142: int32_t Debugger::GetRegisterValue(int regnum) {
How about FPU registers here?

http://codereview.chromium.org/543161/diff/1/52#newcode389
src/mips/simulator-mips.cc:389: //        intptr_t stop_pc = sim_->get_pc() -
Instruction::kInstructionSize;
Code in comments.

http://codereview.chromium.org/543161/diff/1/52#newcode630
src/mips/simulator-mips.cc:630: const_cast<double*>(&Cregisters_[creg]) + 1));
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:630: warning: dereferencing type-punned pointer will
break strict-aliasing rules

This can be circumvented by adding

      'CCFLAGS':      ['-fon-strict-aliasing'],

to the arch:mips of the SConstruct file. However it might be that this could be
done in a more "compiler friendly" way. You might take a look at the bit_cast
template in globals.h and its usage.

However strict aliasing is a problem we are facing in other places. Currently V8
does not compile with GCC 4.4 without specifying -fno-strict-aliasing. See
http://code.google.com/p/v8/issues/detail?id=463

http://codereview.chromium.org/543161/diff/1/52#newcode635
src/mips/simulator-mips.cc:635: return
*(reinterpret_cast<int32_t*>(const_cast<double*>(&Cregisters_[creg])));
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:635: warning: dereferencing type-punned pointer will
break strict-aliasing rules

http://codereview.chromium.org/543161/diff/1/52#newcode651
src/mips/simulator-mips.cc:651: void Simulator::SetFpResult(double result) {
Is this alias for set_fpu_register_double(f0, result) required?

http://codereview.chromium.org/543161/diff/1/52#newcode670
src/mips/simulator-mips.cc:670: UNIMPLEMENTED_();
I think you can put OS::Abort() here and at all the other unaligned accesses.

http://codereview.chromium.org/543161/diff/1/52#newcode887
src/mips/simulator-mips.cc:887: int32_t alu_out;
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:887: warning: 'alu_out' may be used uninitialized in
this function

http://codereview.chromium.org/543161/diff/1/52#newcode1069
src/mips/simulator-mips.cc:1069:
*(reinterpret_cast<double*>(&registers_[rt_reg]));
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:1069: warning: dereferencing type-punned pointer will
break strict-aliasing rules

http://codereview.chromium.org/543161/diff/1/52#newcode1074
src/mips/simulator-mips.cc:1074: registers_[rt_reg];
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:1073: warning: dereferencing type-punned pointer will
break strict-aliasing rules

http://codereview.chromium.org/543161/diff/1/52#newcode1189
src/mips/simulator-mips.cc:1189: int32_t fp_out;
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:1189: warning: 'fp_out' may be used uninitialized in
this function

http://codereview.chromium.org/543161/diff/1/52#newcode1192
src/mips/simulator-mips.cc:1192: int32_t addr;
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:1192: warning: 'addr' may be used uninitialized in
this function

http://codereview.chromium.org/543161/diff/1/52#newcode1238
src/mips/simulator-mips.cc:1238: set_register(31, current_pc + 2*
Instruction::kInstructionSize);
Please use a symbolic constant here.

http://codereview.chromium.org/543161/diff/1/52#newcode1300
src/mips/simulator-mips.cc:1300: //      alu_out = (((*(byte*)addr))<<24)>>24;
Code in comments (two more below).

http://codereview.chromium.org/543161/diff/1/52#newcode1322
src/mips/simulator-mips.cc:1322: fp_out =
*(reinterpret_cast<double*>(&alu_out));
This line gives the following warning with GCC 4.2.4

src/mips/simulator-mips.cc:1322: warning: dereferencing type-punned pointer will
break strict-aliasing rules

http://codereview.chromium.org/543161/diff/1/57
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/1/57#newcode74
src/mips/simulator-mips.h:74: 
I don't understand the first two lines of this comment.

http://codereview.chromium.org/543161/diff/1/57#newcode121
src/mips/simulator-mips.h:121: // Generated code will always use doubles. So we
will only use even registers.
Please make a note that a 32-bit FPU is emulated.

http://codereview.chromium.org/543161/diff/1/57#newcode122
src/mips/simulator-mips.h:122: enum CRegister {
How about FPURegister instead of CRegister?

http://codereview.chromium.org/543161/diff/1/57#newcode136
src/mips/simulator-mips.h:136: 
This comment needs to be changed for MIPS.

http://codereview.chromium.org/543161/diff/1/57#newcode143
src/mips/simulator-mips.h:143: void set_Cregister(int creg, double value);
set_Cregister -> set_fpu_register

http://codereview.chromium.org/543161/diff/1/57#newcode144
src/mips/simulator-mips.h:144: double get_Cregister(int creg) const;
get_Cregister -> get_fpu_register

http://codereview.chromium.org/543161/diff/1/57#newcode145
src/mips/simulator-mips.h:145: // Get 16 high or low bits of the Cregister
Do you mean 16 bits here?

http://codereview.chromium.org/543161/diff/1/57#newcode146
src/mips/simulator-mips.h:146: int32_t get_CregisterHI(int creg) const;
As this is a 32-bit FPU then I will suggest

int32_t get_fpu_register(FPURegister)
double get_fpu_register_double(FPURegister)
void get_fpu_register(FPURegister, int32_t)
void get_fpu_register_double(FPURegister, double)

Where the second variant only allows the even number registers.

http://codereview.chromium.org/543161/diff/1/57#newcode151
src/mips/simulator-mips.h:151: int32_t get_pc() const;
Don't you need the FPU control registers here as well?

http://codereview.chromium.org/543161/diff/1/48
File src/mips/test-interface-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/48#newcode1
src/mips/test-interface-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
How about converting this to a cctest and copy it to test/cctest.

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

http://codereview.chromium.org/543161/diff/1/20#newcode1154
src/objects-inl.h:1154: double HeapNumber::value() {
I guess the reason for this is that on MIPS reading of doubles needs to be
8-byte aligned. A better aproach would be to ensure that that is always the
case.

Erik Corry made an experiment with making the value part of a HeapNumber be
8-byte aligned. It was done to see whether it made a performance improvement,
which it didn't. However it might be what you need for MIPS. The change is still
in Rietveld, see http://codereview.chromium.org/460043. It should be possible to
only use 8-byte alignment on MISP.

Under all circumstances this needs to be factored so there are no architecture
specific #ifdef's in this central file.

http://codereview.chromium.org/543161/diff/1/10
File src/regexp-macro-assembler.cc (right):

http://codereview.chromium.org/543161/diff/1/10#newcode39
src/regexp-macro-assembler.cc:39: #elif V8_TARGET_ARCH_MIPS
I dont think this is required until native RegExp is built for MIPS.

http://codereview.chromium.org/543161/diff/1/3
File src/spaces-inl.h (right):

http://codereview.chromium.org/543161/diff/1/3#newcode287
src/spaces-inl.h:287: 
Please revert this.

http://codereview.chromium.org/543161/diff/1/3#newcode348
src/spaces-inl.h:348: AllocationInfo* alloc_info) {
This looks as a debugging change...

http://codereview.chromium.org/543161/diff/1/2
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/2#newcode1
test/cctest/test-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
You should add this file to test/cctests/SConscript

[Alexandre, 2010-01-26 00:10:13]

Hi,

I have worked on your previous comments.
Thanks again to the reviewers.

This should be much cleaner now. I fear I might have left a few style errors
undetected by tools/presubmit.py. Now I know better Google style guidelines I'll
remove them whenever I see some.

To compile for MIPS we now need the regexp=interpreted option.

Alexandre

http://codereview.chromium.org/543161/diff/1/14
File src/SConscript (right):

http://codereview.chromium.org/543161/diff/1/14#newcode139
src/SConscript:139: mips/regexp-macro-assembler-mips.cc
Removed
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> You don't need mips/regexp-macro-assembler-mips.cc file when only supporting
> interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/59
File src/jsregexp.cc (right):

http://codereview.chromium.org/543161/diff/1/59#newcode4478
src/jsregexp.cc:4478: #elif V8_TARGET_ARCH_MIPS
Forgot to remove. Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This change is not required when only supporting interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/27
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/1/27#newcode116
src/mips/assembler-mips-inl.h:116: return Assembler::target_address_at(pc_);
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Remove two spaces before return.

http://codereview.chromium.org/543161/diff/1/27#newcode141
src/mips/assembler-mips-inl.h:141: Assembler::target_address_at(pc_)));
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Four space indent when splitting line. See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....

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

http://codereview.chromium.org/543161/diff/1/47#newcode178
src/mips/assembler-mips.cc:178: // WII? cf assembler.h 233 :Modes affected by
apply. Depends on arch.
Sorry it was supposed to be removed.
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> I don't understand this comment.

http://codereview.chromium.org/543161/diff/1/47#newcode334
src/mips/assembler-mips.cc:334: 
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> We always try to have excatly two blank lines between functions.

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

http://codereview.chromium.org/543161/diff/1/54#newcode223
src/mips/assembler-mips.h:223: // immediate
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Please start comment with an uppercase letter and end with a period.

http://codereview.chromium.org/543161/diff/1/54#newcode233
src/mips/assembler-mips.h:233: // reg
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/1/54#newcode251
src/mips/assembler-mips.h:251: // Class MemOperand represents a memory operand
in load and store instructions
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/1/54#newcode368
src/mips/assembler-mips.h:368: //  void beql(Register rs, Register rt, int16_t
offset);
Fixed. Removed these instruction. They should not be used.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Code in comment.
> 
> For now I suggest you add TODO(mips) comments like this:
> 
>   // TODO(mips) implement beql(Register rs, Register rt, int16_t offset);
> 
> Normally we try to only have TODO comments in the form of TODO(bugnumber) with
a
> tracking issue, but for this work in progress the general TODO(mips) will be
> fine.

http://codereview.chromium.org/543161/diff/1/54#newcode386
src/mips/assembler-mips.h:386: // IMPORTANT NOTE: Never use the int16_t b(l)cond
version with a branch offset
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Please indent comment to fit the source indention.

http://codereview.chromium.org/543161/diff/1/54#newcode391
src/mips/assembler-mips.h:391: const Operand& rt = Operand(zero_reg), Register
scratch = at);
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Please remove one space.

http://codereview.chromium.org/543161/diff/1/54#newcode393
src/mips/assembler-mips.h:393: const Operand& rt = Operand(zero_reg), Register
scratch = at);
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/1/54#newcode410
src/mips/assembler-mips.h:410: //  INLINE(void jalr  (Register rs);)
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Code in comments.

http://codereview.chromium.org/543161/diff/1/54#newcode548
src/mips/assembler-mips.h:548: void nop()  { sll(zero_reg, zero_reg, 0); }
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Only one space between () and {

http://codereview.chromium.org/543161/diff/1/54#newcode777
src/mips/assembler-mips.h:777: #endif  // V8_ARM_ASSEMBLER_ARM_H_
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> V8_ARM_ASSEMBLER_ARM_H_ -> V8_MIPS_ASSEMBLER_MIPS_H_

http://codereview.chromium.org/543161/diff/1/40
File src/mips/builtins-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/40#newcode93
src/mips/builtins-mips.cc:93: // static void
EnterArgumentsAdaptorFrame(MacroAssembler* masm) {
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Code in comments.

http://codereview.chromium.org/543161/diff/1/33
File src/mips/frames-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/33#newcode82
src/mips/frames-mips.cc:82: UNIMPLEMENTED_();
Forgot to remove.
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Code in comments.

http://codereview.chromium.org/543161/diff/1/33#newcode107
src/mips/frames-mips.cc:107: //  const int arguments =
Smi::cast(GetExpression(0))->value();
Forgot to remove.
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/1/33#newcode116
src/mips/frames-mips.cc:116: // Internal frames have no arguments. The stack
pointer of the
Forgot to remove.
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Ditto.

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

http://codereview.chromium.org/543161/diff/1/46#newcode59
src/mips/macro-assembler-mips.cc:59: UNIMPLEMENTED();
Replaced it.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Maybe use the UNIMPLEMENTED_MIPS macro.

http://codereview.chromium.org/543161/diff/1/46#newcode61
src/mips/macro-assembler-mips.cc:61: // TO_UPGRADE: Use a JAL instead of JALR if
the target is in the pc region and
Changed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Change TO_UPGRADE to TODO(mips).

http://codereview.chromium.org/543161/diff/1/35
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/1/35#newcode55
src/mips/macro-assembler-mips.h:55: class MacroAssembler: public Assembler {
I removed unused functions.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> I suggest you totally remove most of the functions of the macro assembler, and
> add them as needed. There might be some of the ones you have copied that you
> don't need and you might add some which does not make sense for other
> architectures.

http://codereview.chromium.org/543161/diff/1/44
File src/mips/regexp-macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/44#newcode1
src/mips/regexp-macro-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Removed everything.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> You don't need this file when only supporting interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/31
File src/mips/regexp-macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/1/31#newcode1
src/mips/regexp-macro-assembler-mips.h:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
The only thing left now is the declaration of RegExpMacroAssemblerMIPS.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> You don't need this file when only supporting interpreted RegExp.

http://codereview.chromium.org/543161/diff/1/52
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/52#newcode142
src/mips/simulator-mips.cc:142: int32_t Debugger::GetRegisterValue(int regnum) {
Changed
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> How about FPU registers here?

http://codereview.chromium.org/543161/diff/1/52#newcode389
src/mips/simulator-mips.cc:389: //        intptr_t stop_pc = sim_->get_pc() -
Instruction::kInstructionSize;
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Code in comments.

http://codereview.chromium.org/543161/diff/1/52#newcode630
src/mips/simulator-mips.cc:630: const_cast<double*>(&Cregisters_[creg]) + 1));
Casts raising errors now use bit_cast from globals.h .
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:630: warning: dereferencing type-punned pointer
will
> break strict-aliasing rules
> 
> This can be circumvented by adding
> 
>       'CCFLAGS':      ['-fon-strict-aliasing'],
> 
> to the arch:mips of the SConstruct file. However it might be that this could
be
> done in a more "compiler friendly" way. You might take a look at the bit_cast
> template in globals.h and its usage.
> 
> However strict aliasing is a problem we are facing in other places. Currently
V8
> does not compile with GCC 4.4 without specifying -fno-strict-aliasing. See
> http://code.google.com/p/v8/issues/detail?id=463

http://codereview.chromium.org/543161/diff/1/52#newcode635
src/mips/simulator-mips.cc:635: return
*(reinterpret_cast<int32_t*>(const_cast<double*>(&Cregisters_[creg])));
Casts raising errors now use bit_cast from globals.h .
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:635: warning: dereferencing type-punned pointer
will
> break strict-aliasing rules

http://codereview.chromium.org/543161/diff/1/52#newcode651
src/mips/simulator-mips.cc:651: void Simulator::SetFpResult(double result) {
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Is this alias for set_fpu_register_double(f0, result) required?

http://codereview.chromium.org/543161/diff/1/52#newcode670
src/mips/simulator-mips.cc:670: UNIMPLEMENTED_();
Updated
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> I think you can put OS::Abort() here and at all the other unaligned accesses.

http://codereview.chromium.org/543161/diff/1/52#newcode887
src/mips/simulator-mips.cc:887: int32_t alu_out;
This must be because alu_out is used in the default case of the 'execution'
(2nd) switch. Practically it should never be used uninitialized as all
unsupported instructions will raise an error in the 'configuration' (1st)
switch.
I'll fix it by initializing alu_out anyway.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:887: warning: 'alu_out' may be used uninitialized
in
> this function
>

http://codereview.chromium.org/543161/diff/1/52#newcode1069
src/mips/simulator-mips.cc:1069:
*(reinterpret_cast<double*>(&registers_[rt_reg]));
Casts raising errors now use bit_cast from globals.h .
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:1069: warning: dereferencing type-punned pointer
will
> break strict-aliasing rules
>

http://codereview.chromium.org/543161/diff/1/52#newcode1074
src/mips/simulator-mips.cc:1074: registers_[rt_reg];
Casts raising errors now use bit_cast from globals.h .
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:1073: warning: dereferencing type-punned pointer
will
> break strict-aliasing rules
>

http://codereview.chromium.org/543161/diff/1/52#newcode1189
src/mips/simulator-mips.cc:1189: int32_t fp_out;
This is because it is used in 2 different switch statements, with the first
initializing it and the second using it.
Initializing it to fix the issue.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:1189: warning: 'fp_out' may be used uninitialized
in
> this function
>

http://codereview.chromium.org/543161/diff/1/52#newcode1192
src/mips/simulator-mips.cc:1192: int32_t addr;
Same as for fp_out.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:1192: warning: 'addr' may be used uninitialized in
> this function

http://codereview.chromium.org/543161/diff/1/52#newcode1238
src/mips/simulator-mips.cc:1238: set_register(31, current_pc + 2*
Instruction::kInstructionSize);
Added branchReturnOffset constant to constants-mips.h and use it here.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Please use a symbolic constant here.

http://codereview.chromium.org/543161/diff/1/52#newcode1300
src/mips/simulator-mips.cc:1300: //      alu_out = (((*(byte*)addr))<<24)>>24;
Removed
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Code in comments (two more below).

http://codereview.chromium.org/543161/diff/1/52#newcode1322
src/mips/simulator-mips.cc:1322: fp_out =
*(reinterpret_cast<double*>(&alu_out));
Casts raising errors now use bit_cast from globals.h .
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This line gives the following warning with GCC 4.2.4
> 
> src/mips/simulator-mips.cc:1322: warning: dereferencing type-punned pointer
will
> break strict-aliasing rules
>

http://codereview.chromium.org/543161/diff/1/57
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/1/57#newcode74
src/mips/simulator-mips.h:74: 
This was accidentally not removed from previous code.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> I don't understand the first two lines of this comment.

http://codereview.chromium.org/543161/diff/1/57#newcode121
src/mips/simulator-mips.h:121: // Generated code will always use doubles. So we
will only use even registers.
I need more details about that... I implemented some kind of emulation for the
fp coprocessor instructions.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Please make a note that a 32-bit FPU is emulated.

http://codereview.chromium.org/543161/diff/1/57#newcode122
src/mips/simulator-mips.h:122: enum CRegister {
Let's change it.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> How about FPURegister instead of CRegister?

http://codereview.chromium.org/543161/diff/1/57#newcode136
src/mips/simulator-mips.h:136: 
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This comment needs to be changed for MIPS.

http://codereview.chromium.org/543161/diff/1/57#newcode143
src/mips/simulator-mips.h:143: void set_Cregister(int creg, double value);
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> set_Cregister -> set_fpu_register

http://codereview.chromium.org/543161/diff/1/57#newcode144
src/mips/simulator-mips.h:144: double get_Cregister(int creg) const;
Fixed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> get_Cregister -> get_fpu_register

http://codereview.chromium.org/543161/diff/1/57#newcode145
src/mips/simulator-mips.h:145: // Get 16 high or low bits of the Cregister
No it should be 32.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Do you mean 16 bits here?

http://codereview.chromium.org/543161/diff/1/57#newcode146
src/mips/simulator-mips.h:146: int32_t get_CregisterHI(int creg) const;
This makes sense. I update it.
I'm not sure about the use of const_cast in get_fpu_register_double.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> As this is a 32-bit FPU then I will suggest
> 
> int32_t get_fpu_register(FPURegister)
> double get_fpu_register_double(FPURegister)
> void get_fpu_register(FPURegister, int32_t)
> void get_fpu_register_double(FPURegister, double)
> 
> Where the second variant only allows the even number registers.

http://codereview.chromium.org/543161/diff/1/57#newcode151
src/mips/simulator-mips.h:151: int32_t get_pc() const;
Yes I will add them, but instructions using them are not implemented yet, so
I'll add everything together.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Don't you need the FPU control registers here as well?

http://codereview.chromium.org/543161/diff/1/48
File src/mips/test-interface-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/48#newcode1
src/mips/test-interface-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Yes I'll do that.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> How about converting this to a cctest and copy it to test/cctest.

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

http://codereview.chromium.org/543161/diff/1/20#newcode1154
src/objects-inl.h:1154: double HeapNumber::value() {
The reason is indeed that we need 8-byte aligned doubles.
When I met the issue I started to work on a solution increasing the size of
HeapNumber, but I thought this was easier.
I'll have a look at Erik Corry's solution.
I'll delete this for now. This means we can't really use floating point until I
fix it.

On 2010/01/22 20:22:33, Søren Gjesse wrote:
> I guess the reason for this is that on MIPS reading of doubles needs to be
> 8-byte aligned. A better aproach would be to ensure that that is always the
> case.
> 
> Erik Corry made an experiment with making the value part of a HeapNumber be
> 8-byte aligned. It was done to see whether it made a performance improvement,
> which it didn't. However it might be what you need for MIPS. The change is
still
> in Rietveld, see http://codereview.chromium.org/460043. It should be possible
to
> only use 8-byte alignment on MISP.
> 
> Under all circumstances this needs to be factored so there are no architecture
> specific #ifdef's in this central file.

http://codereview.chromium.org/543161/diff/1/10
File src/regexp-macro-assembler.cc (right):

http://codereview.chromium.org/543161/diff/1/10#newcode39
src/regexp-macro-assembler.cc:39: #elif V8_TARGET_ARCH_MIPS
Removed.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> I dont think this is required until native RegExp is built for MIPS.

http://codereview.chromium.org/543161/diff/1/3
File src/spaces-inl.h (right):

http://codereview.chromium.org/543161/diff/1/3#newcode287
src/spaces-inl.h:287: 
Reverted.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> Please revert this.

http://codereview.chromium.org/543161/diff/1/3#newcode348
src/spaces-inl.h:348: AllocationInfo* alloc_info) {
Yes it was, sorry. I remove it.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> This looks as a debugging change...

http://codereview.chromium.org/543161/diff/1/2
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/1/2#newcode1
test/cctest/test-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Added to test/cctests/SConscript.
On 2010/01/22 20:22:33, Søren Gjesse wrote:
> You should add this file to test/cctests/SConscript

[Alexandre, 2010-01-26 18:30:45]

Hi,

Sorry I had a mistake in my script reseting default values in flag-definitions.h
.
I'll correct it and lint errors right now.

Alexandre

http://codereview.chromium.org/543161/diff/4001/5006
File src/flag-definitions.h (left):

http://codereview.chromium.org/543161/diff/4001/5006#oldcode144
src/flag-definitions.h:144: DEFINE_bool(fast_compiler, true,
See before.

http://codereview.chromium.org/543161/diff/4001/5006
File src/flag-definitions.h (right):

http://codereview.chromium.org/543161/diff/4001/5006#newcode121
src/flag-definitions.h:121: DEFINE_string(natives_file, "", "alternative natives
file")
I'm sorry I thought I overwrote with the correct values.
I correct this.

http://codereview.chromium.org/543161/diff/4001/5006#newcode208
src/flag-definitions.h:208: DEFINE_bool(trace_sim, true, "trace simulator
execution")
See before.

[Søren Thygesen Gjesse, 2010-01-26 22:40:55]

Another round of comments, mainly on the MIPS assembler.

http://codereview.chromium.org/543161/diff/4001/5005
File src/bootstrapper.cc (right):

http://codereview.chromium.org/543161/diff/4001/5005#newcode901
src/bootstrapper.cc:901: // Call function using either the runtime object or the
global
Please revert this to remove bootstrapper.cc from this change. I will fix this
on bleeding_edge.

http://codereview.chromium.org/543161/diff/4001/5052
File src/execution.cc (right):

http://codereview.chromium.org/543161/diff/4001/5052#newcode41
src/execution.cc:41: #elif V8_TARGET_ARCH_MIPS
In bleeding_edge this #ifdef block have been moved to "simulator.h"

http://codereview.chromium.org/543161/diff/4001/5044
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/4001/5044#newcode1049
src/mips/assembler-mips.cc:1049: // Arithmetic
These arithmetic instructions should be split into assembler and macro-assembler
versions. E.g. in assembler the addi instruction should ASSERT that the
immediate is 16-bit and just emmit a ADDI instruction. The macro-assembler
should then have an Add macro taking a 32-bit immediate.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1303
src/mips/assembler-mips.cc:1303: Instr instr = SPECIAL | TGE | rs.code()<<21 |
rt.code()<<16 | code<<6;
Why not use the kXxxShift constants from constants-mips.h here and below.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1408
src/mips/assembler-mips.cc:1408: void Assembler::ldc1(CRegister fd, const
MemOperand& src) {
Another macro-assembler macro.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1443
src/mips/assembler-mips.cc:1443: void Assembler::sdc1(CRegister fd, const
MemOperand& src) {
Ditto.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1568
src/mips/assembler-mips.cc:1568: //------------Pseudo-instructions-------------
Pseudo-instructions are macros for the macro-assembler.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1861
src/mips/assembler-mips.cc:1861: //      nop();
Code in comments.

http://codereview.chromium.org/543161/diff/4001/5048
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/4001/5048#newcode132
src/mips/assembler-mips.h:132: struct CRegister {
Rename CRegister to FPURegister like in simulator.

http://codereview.chromium.org/543161/diff/4001/5048#newcode402
src/mips/assembler-mips.h:402: // instruction.
I think these blcond methods should be moved to macro-assembler-mips.* as it is
not MIPS instructions. I suggest the name Branch.

http://codereview.chromium.org/543161/diff/4001/5048#newcode562
src/mips/assembler-mips.h:562: void push(Register src) {
Move to macro-assembler as Push.

http://codereview.chromium.org/543161/diff/4001/5048#newcode567
src/mips/assembler-mips.h:567: void push(Register src, Condition cond, Register
tst1, Register tst2) {
Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode579
src/mips/assembler-mips.h:579: void pop(Register dst) {
Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode583
src/mips/assembler-mips.h:583: void pop() {
Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode598
src/mips/assembler-mips.h:598: void jmp(Label* L) {
Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode701
src/mips/assembler-mips.h:701: // Anyway we could surely implement this
differently.
Consider using the names Immediate, Jump and Register instead of 1, 2 and 3.

http://codereview.chromium.org/543161/diff/4001/5046
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/4001/5046#newcode94
src/mips/constants-mips.h:94: class CRegisters {
Please rename CRegisters to FPURegisters

http://codereview.chromium.org/543161/diff/4001/5046#newcode130
src/mips/constants-mips.h:130: static const int opcode_o   = 26;
Please use the kXxxYyy naming of constants, e.g.

opcode_o -> kOpcodeShift
opcode_l -> kOpcodeBits

and change the type from int to uint32_t.

static const uint32_t kOpcodeBits = 6;
static const uint32_t kOpcodeShift = 21;

Also move enum constants below here. e.g.

static const uint32_t kOpcodeMask = ((1 << kOpcodeBits) - 1) << kOpcodeShift;

Also you might calculate each shift value by adding the bits of the previous
field.

http://codereview.chromium.org/543161/diff/4001/5046#newcode277
src/mips/constants-mips.h:277: enum {
Why are these constants in an enum? How about moving them up to the constants
above?

http://codereview.chromium.org/543161/diff/4001/5046#newcode296
src/mips/constants-mips.h:296: // the 'U' prefix is used to specify unsigned
comparisons.
I think the U prefix should be unsigned_ instead. As an alternative you could
consider he Intel naming, where less and greater are unsigned and above and
below are signed - even though that can also cause confusion.

http://codereview.chromium.org/543161/diff/4001/5046#newcode334
src/mips/constants-mips.h:334: enum C_Condition {
I suggest renaming C_Condition to FPUCondition.

http://codereview.chromium.org/543161/diff/4001/5046#newcode355
src/mips/constants-mips.h:355: // bool InstructionSetsConditionCodes(byte_* ptr)
{
Please update this code sample - the HasS method is not part of this class.
Maybe just remove it.

http://codereview.chromium.org/543161/diff/4001/5046#newcode400
src/mips/constants-mips.h:400: inline int rsField() const {
Rename rsField -> RsField - and for the other methods below.

Maybe ASSERT the instruction type is what is expected. e.g. for this method
ASSERT instruction is immediate or register.

http://codereview.chromium.org/543161/diff/4001/5046#newcode469
src/mips/constants-mips.h:469: inline int32_t Imm16Field() const {
Maybe rename Imm16Field -> ImmediateField?

Maybe ASSERT type is ImmediateInstructionType.

http://codereview.chromium.org/543161/diff/4001/5046#newcode473
src/mips/constants-mips.h:473: inline int32_t Imm26Field() const {
Maybe rename Imm26Field to InstructionIndexField.

Maybe ASSERT type is JumpInstructionType.

http://codereview.chromium.org/543161/diff/4001/5046#newcode484
src/mips/constants-mips.h:484: // Get the encoding type of the instruction.
How about changing the type into an enum with the values

kImmediateType
kJumpType
kRegisterType

or some other names which provide more info than 1,2 and 3?

http://codereview.chromium.org/543161/diff/4001/5046#newcode506
src/mips/constants-mips.h:506: 
Name constants like kXxxYyy.

[Alexandre, 2010-01-27 23:28:20]

Hi,

I've worked on your comments.
The big work in this change. was to move the code from the Assembler to the
MacroAssembler.
There is also a new test for the assembler. However I actually use a
MacroAssembler to use the Branch instructions. Otherwise only Assembler
instructions are used.

I'll keep working on all this and start implementing basic code for functions,
as you said on the v8-users list.

Regards,

Alexandre.

http://codereview.chromium.org/543161/diff/4001/5005
File src/bootstrapper.cc (right):

http://codereview.chromium.org/543161/diff/4001/5005#newcode901
src/bootstrapper.cc:901: // Call function using either the runtime object or the
global
Reverted
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Please revert this to remove bootstrapper.cc from this change. I will fix this
> on bleeding_edge.

http://codereview.chromium.org/543161/diff/4001/5052
File src/execution.cc (right):

http://codereview.chromium.org/543161/diff/4001/5052#newcode41
src/execution.cc:41: #elif V8_TARGET_ARCH_MIPS
Ok. I don't have the simulator.h file yet. I'll change this as soon as I do.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> In bleeding_edge this #ifdef block have been moved to "simulator.h"

http://codereview.chromium.org/543161/diff/4001/5044
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/4001/5044#newcode1049
src/mips/assembler-mips.cc:1049: // Arithmetic
I'll simplify all instructions and move the higher level to the macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> These arithmetic instructions should be split into assembler and
macro-assembler
> versions. E.g. in assembler the addi instruction should ASSERT that the
> immediate is 16-bit and just emmit a ADDI instruction. The macro-assembler
> should then have an Add macro taking a 32-bit immediate.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1303
src/mips/assembler-mips.cc:1303: Instr instr = SPECIAL | TGE | rs.code()<<21 |
rt.code()<<16 | code<<6;
They were implemented later than this code. I'll replace them.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Why not use the kXxxShift constants from constants-mips.h here and below.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1408
src/mips/assembler-mips.cc:1408: void Assembler::ldc1(CRegister fd, const
MemOperand& src) {
Originally it wasn't. Anyway I have to remove the alignment stuff since the
solution of emulating 4-byte alignment access is not ok.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Another macro-assembler macro.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1443
src/mips/assembler-mips.cc:1443: void Assembler::sdc1(CRegister fd, const
MemOperand& src) {
Macro removed.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1568
src/mips/assembler-mips.cc:1568: //------------Pseudo-instructions-------------
Moved to the macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Pseudo-instructions are macros for the macro-assembler.

http://codereview.chromium.org/543161/diff/4001/5044#newcode1861
src/mips/assembler-mips.cc:1861: //      nop();
Well it was more like a helper to remind which instruction we are generating. I
change it to look more like comments.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Code in comments.

http://codereview.chromium.org/543161/diff/4001/5048
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/4001/5048#newcode132
src/mips/assembler-mips.h:132: struct CRegister {
Renamed.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Rename CRegister to FPURegister like in simulator.

http://codereview.chromium.org/543161/diff/4001/5048#newcode402
src/mips/assembler-mips.h:402: // instruction.
Moved to macro-assembler. Renamed to Branch / BranchAndLink.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> I think these blcond methods should be moved to macro-assembler-mips.* as it
is
> not MIPS instructions. I suggest the name Branch.

http://codereview.chromium.org/543161/diff/4001/5048#newcode562
src/mips/assembler-mips.h:562: void push(Register src) {
Moved to macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Move to macro-assembler as Push.

http://codereview.chromium.org/543161/diff/4001/5048#newcode567
src/mips/assembler-mips.h:567: void push(Register src, Condition cond, Register
tst1, Register tst2) {
Moved to macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode579
src/mips/assembler-mips.h:579: void pop(Register dst) {
Moved to macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode583
src/mips/assembler-mips.h:583: void pop() {
Moved to macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode598
src/mips/assembler-mips.h:598: void jmp(Label* L) {
Moved to macro-assembler.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/4001/5048#newcode701
src/mips/assembler-mips.h:701: // Anyway we could surely implement this
differently.
Renamed to GenInstrRegister, GenInstrImmediate, GenInstrImm26.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Consider using the names Immediate, Jump and Register instead of 1, 2 and 3.

http://codereview.chromium.org/543161/diff/4001/5046
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/4001/5046#newcode94
src/mips/constants-mips.h:94: class CRegisters {
I don't know why it's still there. I rename it.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Please rename CRegisters to FPURegisters

http://codereview.chromium.org/543161/diff/4001/5046#newcode130
src/mips/constants-mips.h:130: static const int opcode_o   = 26;
Renamed and modified.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Please use the kXxxYyy naming of constants, e.g.
> 
> opcode_o -> kOpcodeShift
> opcode_l -> kOpcodeBits
> 
> and change the type from int to uint32_t.
> 
> static const uint32_t kOpcodeBits = 6;
> static const uint32_t kOpcodeShift = 21;
> 
> Also move enum constants below here. e.g.
> 
> static const uint32_t kOpcodeMask = ((1 << kOpcodeBits) - 1) << kOpcodeShift;
> 
> Also you might calculate each shift value by adding the bits of the previous
> field.

http://codereview.chromium.org/543161/diff/4001/5046#newcode277
src/mips/constants-mips.h:277: enum {
Done.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Why are these constants in an enum? How about moving them up to the constants
> above?

http://codereview.chromium.org/543161/diff/4001/5046#newcode296
src/mips/constants-mips.h:296: // the 'U' prefix is used to specify unsigned
comparisons.
I don't like the confusion in the Intel naming. Also these condition are often
used so the short U prefix is quite useful... Anyway if needed I can change it.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> I think the U prefix should be unsigned_ instead. As an alternative you could
> consider he Intel naming, where less and greater are unsigned and above and
> below are signed - even though that can also cause confusion.

http://codereview.chromium.org/543161/diff/4001/5046#newcode334
src/mips/constants-mips.h:334: enum C_Condition {
Yes. This fits better with the new names.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> I suggest renaming C_Condition to FPUCondition.

http://codereview.chromium.org/543161/diff/4001/5046#newcode355
src/mips/constants-mips.h:355: // bool InstructionSetsConditionCodes(byte_* ptr)
{
Removed. I don't know what it was still doing here.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Please update this code sample - the HasS method is not part of this class.
> Maybe just remove it.

http://codereview.chromium.org/543161/diff/4001/5046#newcode400
src/mips/constants-mips.h:400: inline int rsField() const {
I rename fields accessors for more coherence.
I also add a few asserts to control fields access.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Rename rsField -> RsField - and for the other methods below.
> 
> Maybe ASSERT the instruction type is what is expected. e.g. for this method
> ASSERT instruction is immediate or register.

http://codereview.chromium.org/543161/diff/4001/5046#newcode469
src/mips/constants-mips.h:469: inline int32_t Imm16Field() const {
Well I like the Imm16 because it says how many bits it uses, but I don't care
too much it's used very locally.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Maybe rename Imm16Field -> ImmediateField?
> 
> Maybe ASSERT type is ImmediateInstructionType.

http://codereview.chromium.org/543161/diff/4001/5046#newcode473
src/mips/constants-mips.h:473: inline int32_t Imm26Field() const {
Here I like it to be similar to Imm16Field instead of a long name. But again I
don't care too much.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Maybe rename Imm26Field to InstructionIndexField.
> 
> Maybe ASSERT type is JumpInstructionType.

http://codereview.chromium.org/543161/diff/4001/5046#newcode484
src/mips/constants-mips.h:484: // Get the encoding type of the instruction.
Yes good idea I do that.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> How about changing the type into an enum with the values
> 
> kImmediateType
> kJumpType
> kRegisterType
> 
> or some other names which provide more info than 1,2 and 3?

http://codereview.chromium.org/543161/diff/4001/5046#newcode506
src/mips/constants-mips.h:506: 
Renamed.
On 2010/01/26 22:40:55, Søren Gjesse wrote:
> Name constants like kXxxYyy.

http://codereview.chromium.org/543161/diff/4001/5034
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/4001/5034#newcode560
src/mips/disasm-mips.cc:560: //        case TGEI:
Removed this commented code.

[Alexandre, 2010-01-27 23:40:42]

Removed a line forgotten.

http://codereview.chromium.org/543161/diff/8002/9044
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9044#newcode456
src/mips/constants-mips.h:456: ASSERT(instrType() == kRegisterType ||
instrType() == kImmediateType);
Accidentally left this code. Removed.

[Søren Thygesen Gjesse, 2010-01-28 16:25:40]

Thank you for addressing the changes from yesterday. I think it's starting to
look good. Also the assembler tests are fine.

With the following patch

Index: test/cctest/cctest.status
===================================================================
--- test/cctest/cctest.status	(revision 3220)
+++ test/cctest/cctest.status	(working copy)
@@ -73,3 +73,23 @@
 # the JavaScript stacks are separate.
 test-api/ExceptionOrder: FAIL
 test-api/TryCatchInTryFinally: FAIL
+
+[ $arch == mips ]
+test-accessors: SKIP
+test-alloc: SKIP
+test-api: SKIP
+test-compiler: SKIP
+test-debug: SKIP
+test-decls: SKIP
+test-func-name-inference: SKIP
+test-heap: SKIP
+test-heap-profiler: SKIP
+test-log: SKIP
+test-log-utils: SKIP
+test-mark-compact: SKIP
+test-regexp: SKIP
+test-serialize: SKIP
+test-sockets: SKIP
+test-strings: SKIP
+test-threads: SKIP
+test-thread-termination: SKIP

I can now run

tools/test.py --simulator=mips cctest

and get

47 parsing tests (5 of those being the MIPS assembler tests).
Here are some more comments.

Also please look into removing the changes to src/jump-target.* - they should
not be required to get the assembler,
disassembler and simulator in.

As CodeGen is not doing anything right now please trim it even further so that
it just compiles. I think it should be possible to get rid of the virtual-frame*
and register-allocator* for just getting the assembler, disassembler and
simulator in.

Thanks,
Søren

http://codereview.chromium.org/543161/diff/8002/9052
File SConstruct (right):

http://codereview.chromium.org/543161/diff/8002/9052#newcode849
SConstruct:849: options['profilingsupport'] = 'on'
If you add the following here you don't need to pass regexp=interpreted when
running SCons.

  if options['arch'] == 'mips':
    if ('regexp' in ARGUMENTS) and options['regexp'] == 'native':
      # Print a warning if native regexp is specified for mips
      print "Warning: forcing regexp to interpreted for mips"
    options['regexp'] = 'interpreted'

http://codereview.chromium.org/543161/diff/8002/9017
File src/code-stubs.h (right):

http://codereview.chromium.org/543161/diff/8002/9017#newcode63
src/code-stubs.h:63: #define CODE_STUB_LIST_ARM(V)
Please revert this file, as these changes are not required to get the assembler,
disassembler and simulator in.

http://codereview.chromium.org/543161/diff/8002/9006
File src/codegen.h (right):

http://codereview.chromium.org/543161/diff/8002/9006#newcode151
src/codegen.h:151: #ifndef V8_TARGET_ARCH_MIPS
Please revert this change, as it is not required to get the assembler,
disassembler and simulator in.

http://codereview.chromium.org/543161/diff/8002/9006#newcode156
src/codegen.h:156: #endif
Please revert this change, as it is not required to get the assembler,
disassembler and simulator in.

Remove the function definition in codegen-mips-inl.h as well.

http://codereview.chromium.org/543161/diff/8002/9018
File src/heap.h (right):

http://codereview.chromium.org/543161/diff/8002/9018#newcode157
src/heap.h:157: #if V8_TARGET_ARCH_MIPS
Please revert this file, as it is not required to get the assembler,
disassembler and simulator in.

http://codereview.chromium.org/543161/diff/8002/9022
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/8002/9022#newcode201
src/mips/assembler-mips-inl.h:201: return ((Assembler::instr_at(pc_) &
kOpcodeMask) == SPECIAL) &&
Please add an extra set of () around the && expression, like this (x && y) || z.
I know the operator precedence will take care of that, but the extra ()'s makes
it more readable, and for multiline expressions it will provide indention for
redability. Also remove the outer ()'s as thay are not needed for the return
statement.

http://codereview.chromium.org/543161/diff/8002/9042
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9042#newcode499
src/mips/assembler-mips.cc:499: int16_t  j) {
Two spaces before j. Also 4 more times below.

http://codereview.chromium.org/543161/diff/8002/9042#newcode602
src/mips/assembler-mips.cc:602: ASSERT(is_int16(offset));  // We check the
offset can be used in a branch.
Maybe the ASSERT here and in all the other instruction generation calling
GenInstrImmediate, GenInstrRegister and GenInstrImm26 can be moved to
GenInstrImmediate, GenInstrRegister and GenInstrImm26. They all seem to do the
same for each instruction encoding format.

http://codereview.chromium.org/543161/diff/8002/9020
File src/mips/codegen-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/8002/9020#newcode69
src/mips/codegen-mips-inl.h:69: void DeferredCode::Branch(Condition cond,
Register src1, const Operand& src2) {
Remove this function when codegen.h is reverted.

http://codereview.chromium.org/543161/diff/8002/9044
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9044#newcode84
src/mips/constants-mips.h:84: static int32_t max_val() { return kMaxValue_; }
Do you need max_val and min_val? How about just using kMaxInt and kMinInt from
globals.h. If you would like to have them here I suggest defining

static const int32_t kMaxValue = 0x7fffffff;
static const int32_t kMinValue = 0x80000000;

public and use Registers::kMaxValue and Registers::kMinValue to access them.

http://codereview.chromium.org/543161/diff/8002/9041
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9041#newcode337
src/mips/macro-assembler-mips.cc:337: lui(rd, (HIMask & j.imm32_)>>16);
Spaces around >>.

http://codereview.chromium.org/543161/diff/8002/9041#newcode339
src/mips/macro-assembler-mips.cc:339: lui(rd, (HIMask & j.imm32_)>>16);
Ditto.

http://codereview.chromium.org/543161/diff/8002/9041#newcode363
src/mips/macro-assembler-mips.cc:363: } else if (gen2instr) {
Here you are preparing the instruction stream for patching the immediate value.
However, no relocation info is recorded, so how do we end up finding this code
again to patch it?

http://codereview.chromium.org/543161/diff/8002/9041#newcode367
src/mips/macro-assembler-mips.cc:367: if (is_int16(j.imm32_)) {
If we need this anyway, then please refactor this code as it is the same as in
the "else if" above.

http://codereview.chromium.org/543161/diff/8002/9041#newcode462
src/mips/macro-assembler-mips.cc:462: case cc_always:
Do you want the li(r2, rt) abowe executed in case of cc_always?

The same goes for the other branch functions below.

http://codereview.chromium.org/543161/diff/8002/9041#newcode541
src/mips/macro-assembler-mips.cc:541: bne(scratch, zero_reg, branch_offset(L,
false)>>2);
Consider creating a function calculating "branch_offset(L, false) >> 2".

http://codereview.chromium.org/543161/diff/8002/9030
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9030#newcode40
src/mips/macro-assembler-mips.h:40: // Register at is used for insruction
generation. So it is not safe to use it
insruction -> instruction

http://codereview.chromium.org/543161/diff/8002/9030#newcode109
src/mips/macro-assembler-mips.h:109: void jmp(Label* L) {
jmp -> Jump

http://codereview.chromium.org/543161/diff/8002/9030#newcode132
src/mips/macro-assembler-mips.h:132: #define DEFINE_INSTRUCTION_MACRO_(instr)   
                                   \
Consider removing _MACRO_.

http://codereview.chromium.org/543161/diff/8002/9030#newcode141
src/mips/macro-assembler-mips.h:141: #define DEFINE_INSTRUCTION_MACRO_2(instr)  
                                   \
Ditto.

http://codereview.chromium.org/543161/diff/8002/9030#newcode166
src/mips/macro-assembler-mips.h:166: #undef DEFINE_INSTRUCTION_MACRO_
Also undef DEFINE_INSTRUCTION_MACRO_2

http://codereview.chromium.org/543161/diff/8002/9030#newcode171
src/mips/macro-assembler-mips.h:171: void mov(Register rd, Register rt) {
or_(rd, rt, zero_reg); }
The macros in the macro assembler are usually named XxxYxx to distinguish them
from real instructions. e.g.

push -> Push
multi_push -> PushMultiple

However commonly used pseudo-instructions (like li) should probably keep their
most common name, your call.

http://codereview.chromium.org/543161/diff/8002/9049
File src/mips/register-allocator-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/8002/9049#newcode28
src/mips/register-allocator-mips-inl.h:28: #ifndef
V8_IA32_REGISTER_ALLOCATOR_IA32_INL_H_
IA32 -> MIPS (times 3=

[antonm, 2010-01-28 18:05:50]

http://codereview.chromium.org/543161/diff/8002/9042
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9042#newcode1234
src/mips/assembler-mips.cc:1234: //      nop
I think comments should be indented as well

http://codereview.chromium.org/543161/diff/8002/9034
File src/mips/cpu-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9034#newcode51
src/mips/cpu-mips.cc:51: //  int cacheflush(char *addr, int nbytes, int cache);
nit: comment formatting

http://codereview.chromium.org/543161/diff/8002/9032
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9032#newcode69
src/mips/disasm-mips.cc:69: #define UNSUPPORTED() printf("Unsupported
instruction.\n");
PrintF?

http://codereview.chromium.org/543161/diff/8002/9032#newcode758
src/mips/disasm-mips.cc:758: //  int instruction_bits =
*(reinterpret_cast<int*>(instruction));
remove those intstructions?

http://codereview.chromium.org/543161/diff/8002/9045
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9045#newcode1494
src/mips/simulator-mips.cc:1494: //
nit: strange comment, here and below

http://codereview.chromium.org/543161/diff/8002/9029
File src/mips/virtual-frame-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9029#newcode210
src/mips/virtual-frame-mips.cc:210: return Result();
if it's valid implementation, maybe inline it?

http://codereview.chromium.org/543161/diff/8002/9047
File src/mips/virtual-frame-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9047#newcode250
src/mips/virtual-frame-mips.h:250: // Restore the esi register from the value of
the context frame
esi, ia32-ism?

[Alexandre, 2010-01-28 22:32:05]

Hi,

I have done some fixes. I also removed the MIPS code in jump-target.* as you
suggested.

Alexandre

http://codereview.chromium.org/543161/diff/8002/9052
File SConstruct (right):

http://codereview.chromium.org/543161/diff/8002/9052#newcode849
SConstruct:849: options['profilingsupport'] = 'on'
Added this fix.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> If you add the following here you don't need to pass regexp=interpreted when
> running SCons.
> 
>   if options['arch'] == 'mips':
>     if ('regexp' in ARGUMENTS) and options['regexp'] == 'native':
>       # Print a warning if native regexp is specified for mips
>       print "Warning: forcing regexp to interpreted for mips"
>     options['regexp'] = 'interpreted'
>

http://codereview.chromium.org/543161/diff/8002/9017
File src/code-stubs.h (right):

http://codereview.chromium.org/543161/diff/8002/9017#newcode63
src/code-stubs.h:63: #define CODE_STUB_LIST_ARM(V)
Removed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Please revert this file, as these changes are not required to get the
assembler,
> disassembler and simulator in.

http://codereview.chromium.org/543161/diff/8002/9006
File src/codegen.h (right):

http://codereview.chromium.org/543161/diff/8002/9006#newcode151
src/codegen.h:151: #ifndef V8_TARGET_ARCH_MIPS
Removed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Please revert this change, as it is not required to get the assembler,
> disassembler and simulator in.

http://codereview.chromium.org/543161/diff/8002/9006#newcode156
src/codegen.h:156: #endif
Removed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Please revert this change, as it is not required to get the assembler,
> disassembler and simulator in.
> 
> Remove the function definition in codegen-mips-inl.h as well.

http://codereview.chromium.org/543161/diff/8002/9018
File src/heap.h (right):

http://codereview.chromium.org/543161/diff/8002/9018#newcode157
src/heap.h:157: #if V8_TARGET_ARCH_MIPS
Removed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Please revert this file, as it is not required to get the assembler,
> disassembler and simulator in.

http://codereview.chromium.org/543161/diff/8002/9022
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/8002/9022#newcode201
src/mips/assembler-mips-inl.h:201: return ((Assembler::instr_at(pc_) &
kOpcodeMask) == SPECIAL) &&
I think you misread these lines. The expression format is x && (y || z).
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Please add an extra set of () around the && expression, like this (x && y) ||
z.
> I know the operator precedence will take care of that, but the extra ()'s
makes
> it more readable, and for multiline expressions it will provide indention for
> redability. Also remove the outer ()'s as thay are not needed for the return
> statement.

http://codereview.chromium.org/543161/diff/8002/9042
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9042#newcode499
src/mips/assembler-mips.cc:499: int16_t  j) {
Removed one space each time.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Two spaces before j. Also 4 more times below.

http://codereview.chromium.org/543161/diff/8002/9042#newcode602
src/mips/assembler-mips.cc:602: ASSERT(is_int16(offset));  // We check the
offset can be used in a branch.
I reworked these assert statements. They are not needed in the branch
instructions and the checks on registers can be done in the GenInstrxxx
functions. With the previous move from assembler code to MacroAssembler I could
also simplify the GenInstrxxx functions.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Maybe the ASSERT here and in all the other instruction generation calling
> GenInstrImmediate, GenInstrRegister and GenInstrImm26 can be moved to
> GenInstrImmediate, GenInstrRegister and GenInstrImm26. They all seem to do the
> same for each instruction encoding format.

http://codereview.chromium.org/543161/diff/8002/9042#newcode1234
src/mips/assembler-mips.cc:1234: //      nop
Yes. Fixed.
On 2010/01/28 18:05:50, antonm wrote:
> I think comments should be indented as well

http://codereview.chromium.org/543161/diff/8002/9020
File src/mips/codegen-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/8002/9020#newcode69
src/mips/codegen-mips-inl.h:69: void DeferredCode::Branch(Condition cond,
Register src1, const Operand& src2) {
Removed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Remove this function when codegen.h is reverted.

http://codereview.chromium.org/543161/diff/8002/9044
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9044#newcode84
src/mips/constants-mips.h:84: static int32_t max_val() { return kMaxValue_; }
Well they can be removed if we are sure that globals.h kMax/MinInt won't change.
For example if we decide to adapt them for 64bits architectures.
For now I keep them and rename them as you suggested. It's true that we don't
need them private with accessors.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Do you need max_val and min_val? How about just using kMaxInt and kMinInt from
> globals.h. If you would like to have them here I suggest defining
> 
> static const int32_t kMaxValue = 0x7fffffff;
> static const int32_t kMinValue = 0x80000000;
> 
> public and use Registers::kMaxValue and Registers::kMinValue to access them.

http://codereview.chromium.org/543161/diff/8002/9034
File src/mips/cpu-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9034#newcode51
src/mips/cpu-mips.cc:51: //  int cacheflush(char *addr, int nbytes, int cache);
removed the commented code and corrected the other line.
On 2010/01/28 18:05:50, antonm wrote:
> nit: comment formatting

http://codereview.chromium.org/543161/diff/8002/9032
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9032#newcode69
src/mips/disasm-mips.cc:69: #define UNSUPPORTED() printf("Unsupported
instruction.\n");
Fixed.
On 2010/01/28 18:05:50, antonm wrote:
> PrintF?

http://codereview.chromium.org/543161/diff/8002/9032#newcode758
src/mips/disasm-mips.cc:758: //  int instruction_bits =
*(reinterpret_cast<int*>(instruction));
Removed.
On 2010/01/28 18:05:50, antonm wrote:
> remove those intstructions?

http://codereview.chromium.org/543161/diff/8002/9041
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9041#newcode337
src/mips/macro-assembler-mips.cc:337: lui(rd, (HIMask & j.imm32_)>>16);
Fixed all.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Spaces around >>.

http://codereview.chromium.org/543161/diff/8002/9041#newcode339
src/mips/macro-assembler-mips.cc:339: lui(rd, (HIMask & j.imm32_)>>16);
Fixed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/8002/9041#newcode363
src/mips/macro-assembler-mips.cc:363: } else if (gen2instr) {
Actually this code structure comes from older code. The code changed and removed
the need for 2 separate if / else but I didn't correct this function. I rework
all this.
The gen2instr boolean is here because we may want to be sure of how many
instructions are generated, for example if we want to check the length of some
code sequence.
2010/01/28 16:25:40, Søren Gjesse wrote:
> Here you are preparing the instruction stream for patching the immediate
value.
> However, no relocation info is recorded, so how do we end up finding this code
> again to patch it?

http://codereview.chromium.org/543161/diff/8002/9041#newcode367
src/mips/macro-assembler-mips.cc:367: if (is_int16(j.imm32_)) {
See previous comment.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> If we need this anyway, then please refactor this code as it is the same as in
> the "else if" above.

http://codereview.chromium.org/543161/diff/8002/9041#newcode462
src/mips/macro-assembler-mips.cc:462: case cc_always:
We don't need it. Fixed.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Do you want the li(r2, rt) abowe executed in case of cc_always?
> 
> The same goes for the other branch functions below.

http://codereview.chromium.org/543161/diff/8002/9041#newcode541
src/mips/macro-assembler-mips.cc:541: bne(scratch, zero_reg, branch_offset(L,
false)>>2);
Added shifted_branch_offset (rename it to sll2_branch_offset?) in the Assembler.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Consider creating a function calculating "branch_offset(L, false) >> 2".

http://codereview.chromium.org/543161/diff/8002/9030
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9030#newcode40
src/mips/macro-assembler-mips.h:40: // Register at is used for insruction
generation. So it is not safe to use it
Corrected
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> insruction -> instruction

http://codereview.chromium.org/543161/diff/8002/9030#newcode109
src/mips/macro-assembler-mips.h:109: void jmp(Label* L) {
jmp is used in architecture independent files (codegen.cc and fast-codegen.cc).
I could move it the Assembler.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> jmp -> Jump

http://codereview.chromium.org/543161/diff/8002/9030#newcode132
src/mips/macro-assembler-mips.h:132: #define DEFINE_INSTRUCTION_MACRO_(instr)   
                                   \
Yes. the suffix was there because I used DEFINE_INSTRUCTION in the Assembler
(you didn't get this version).
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Consider removing _MACRO_.

http://codereview.chromium.org/543161/diff/8002/9030#newcode141
src/mips/macro-assembler-mips.h:141: #define DEFINE_INSTRUCTION_MACRO_2(instr)  
                                   \
Same.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/8002/9030#newcode166
src/mips/macro-assembler-mips.h:166: #undef DEFINE_INSTRUCTION_MACRO_
Added.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> Also undef DEFINE_INSTRUCTION_MACRO_2

http://codereview.chromium.org/543161/diff/8002/9030#newcode171
src/mips/macro-assembler-mips.h:171: void mov(Register rd, Register rt) {
or_(rd, rt, zero_reg); }
Updated a few names to camel case. 
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> The macros in the macro assembler are usually named XxxYxx to distinguish them
> from real instructions. e.g.
> 
> push -> Push
> multi_push -> PushMultiple
> 
> However commonly used pseudo-instructions (like li) should probably keep their
> most common name, your call.

http://codereview.chromium.org/543161/diff/8002/9049
File src/mips/register-allocator-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/8002/9049#newcode28
src/mips/register-allocator-mips-inl.h:28: #ifndef
V8_IA32_REGISTER_ALLOCATOR_IA32_INL_H_
Corrected.
On 2010/01/28 16:25:40, Søren Gjesse wrote:
> IA32 -> MIPS (times 3=

http://codereview.chromium.org/543161/diff/8002/9045
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9045#newcode1494
src/mips/simulator-mips.cc:1494: //
Removed.
On 2010/01/28 18:05:50, antonm wrote:
> nit: strange comment, here and below

http://codereview.chromium.org/543161/diff/8002/9029
File src/mips/virtual-frame-mips.cc (right):

http://codereview.chromium.org/543161/diff/8002/9029#newcode210
src/mips/virtual-frame-mips.cc:210: return Result();
Set to UNIMPLEMENTED_().
On 2010/01/28 18:05:50, antonm wrote:
> if it's valid implementation, maybe inline it?

http://codereview.chromium.org/543161/diff/8002/9047
File src/mips/virtual-frame-mips.h (right):

http://codereview.chromium.org/543161/diff/8002/9047#newcode250
src/mips/virtual-frame-mips.h:250: // Restore the esi register from the value of
the context frame
Error from code copy. Btw the error still exists in virtual-frame-arm.h in
bleeding_edge.
On 2010/01/28 18:05:50, antonm wrote:
> esi, ia32-ism?

[antonm, 2010-01-29 10:47:11]

Alexander, apparently you've updated svn and that rendered code review almost
impossible---there are difference which were just brought by update.

I know that's quite irritating, but couldn't you either revert to the previous
revision or start new review?

http://codereview.chromium.org/543161/diff/3003/12038
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12038#newcode1162
src/mips/assembler-mips.cc:1162: // 
Memory::Address_at(target_address_address_at(pc)) = target;
sorry for nit picking, once again comments at the start of the line.  Here and
below.

[Søren Thygesen Gjesse, 2010-01-29 12:58:07]

Hi,

Thanks for addressing the comments. The assembler is looking quite nice by now.

Some more comments below.

As Anton mentioned you updated some of the files to head. As far as I can see
the following files are at r3737:

  src/register-allocator.h    (revision 3737)
  src/codegen-inl.h   (revision 3737)
  src/flag-definitions.h      (revision 3737)
  src/codegen.h       (revision 3737)
  src/virtual-frame.h (revision 3737)
  src/objects.h       (revision 3737)

Whereas the rest is still at 3220:

  src/SConscript      (revision 3220)
  src/macro-assembler.h       (revision 3220)
  src/assembler.h     (revision 3220)
  src/v8.cc   (revision 3220)
  src/platform-linux.cc       (revision 3220)
  src/frames-inl.h    (revision 3220)
  src/objects-inl.h   (revision 3220)
  src/register-allocator-inl.h        (revision 3220)
  src/execution.cc    (revision 3220)
  src/globals.h       (revision 3220)
  SConstruct  (revision 3220)
  test/cctest/test-regexp.cc  (revision 3220)
  test/cctest/SConscript      (revision 3220)

It looks as if you have been hit by a failure in Rietveld (the codereview tool).
It suffers from internal errors from time to time. If you click on the diff from
patch set 4 to 5 on e.g. src/codegen.h you will get the message "Upload in
progress.", which is an indication that your "gcl upload ..." did not complete.

I think this will be a good point for you to update your workspace to head (you
probably already did that) and start a new code review cycle.

I have sent you a further trimmed version of src/mips/codegen* on email, that I
suggest you use in order to have as little as possible in these files.

Regards,
Søren

http://codereview.chromium.org/543161/diff/3003/12018
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/3003/12018#newcode203
src/mips/assembler-mips-inl.h:203: ((Assembler::instr_at(pc_) &
kFunctionFieldMask) == JALR));
You need a one character indent of the last line of the expression.

http://codereview.chromium.org/543161/diff/3003/12038
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12038#newcode998
src/mips/assembler-mips.cc:998: cc << 8 | 3 << 4 | cond;
4 character indent here. Perhaps if splitting just after the = the rest can be
on one line.

http://codereview.chromium.org/543161/diff/3003/12040
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12040#newcode35
src/mips/constants-mips.h:35: #define UNIMPLEMENTED_()                          
                        \
I suggest naming this macro UNIMPLEMENTED_MIPS

http://codereview.chromium.org/543161/diff/3003/12040#newcode38
src/mips/constants-mips.h:38: #define UNSUPPORTED_()
v8::internal::PrintF("Unsupported instruction.\n")
and UNSUPPORTED_MIPS

http://codereview.chromium.org/543161/diff/3003/12040#newcode499
src/mips/constants-mips.h:499: // Instructions are read of out a code stream.
The only way to get a
Rename isXXX -> IsXXX

http://codereview.chromium.org/543161/diff/3003/12028
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12028#newcode125
src/mips/disasm-mips.cc:125: // Each of these functions decodes one particular
instruction type.
How about using names instead of numbers?

http://codereview.chromium.org/543161/diff/3003/12028#newcode164
src/mips/disasm-mips.cc:164: 
Two empty lines between methods.

http://codereview.chromium.org/543161/diff/3003/12028#newcode235
src/mips/disasm-mips.cc:235: out_buffer_pos_ += v8i::OS::SNPrintF(out_buffer_ +
out_buffer_pos_,
Either aling with ( or use 4 space indent. If splitting after += the rest might
fit on one line.

http://codereview.chromium.org/543161/diff/3003/12028#newcode246
src/mips/disasm-mips.cc:246: out_buffer_pos_ += v8i::OS::SNPrintF(out_buffer_ +
out_buffer_pos_,
Ditto.

http://codereview.chromium.org/543161/diff/3003/12019
File src/mips/jump-target-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12019#newcode50
src/mips/jump-target-mips.cc:50: void JumpTarget::DoBranch(Condition cc, Hint
ignored,
Remove the two Register parameters for code to compile.

http://codereview.chromium.org/543161/diff/3003/12026
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12026#newcode256
src/mips/macro-assembler-mips.h:256: //
---------------------------------------------------------------------------
Please remove all the which are specific to V8 code generation and add them on a
needed basis.

http://codereview.chromium.org/543161/diff/3003/12026#newcode490
src/mips/macro-assembler-mips.h:490: class CodePatcher {
Just remove the code patcher for now and add it back when needed.

http://codereview.chromium.org/543161/diff/3003/12035
File src/mips/regexp-macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12035#newcode1
src/mips/regexp-macro-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Please remove this file as it is no longer needed.

http://codereview.chromium.org/543161/diff/3003/12022
File src/mips/regexp-macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12022#newcode1
src/mips/regexp-macro-assembler-mips.h:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Please remove this file as it is no longer needed.

http://codereview.chromium.org/543161/diff/3003/12041
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12041#newcode1231
src/mips/simulator-mips.cc:1231: //        case TGEI:
Code in comment.

http://codereview.chromium.org/543161/diff/3003/12041#newcode1252
src/mips/simulator-mips.cc:1252: //      case TGEI:
Code in comment.

http://codereview.chromium.org/543161/diff/3003/12044
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12044#newcode208
src/mips/simulator-mips.h:208: // Executing is handled based on the instruction
type.
How about using names instead of numbers here?

http://codereview.chromium.org/543161/diff/3003/12001
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12001#newcode180
test/cctest/test-assembler-mips.cc:180: //  __ mfhi(v1);
Code in comments, and below.

http://codereview.chromium.org/543161/diff/3003/12049
File test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/543161/diff/3003/12049#newcode1
test/cctest/test-regexp.cc:1: // Copyright 2008 the V8 project authors. All
rights reserved.
You can revert this file when src/mips/regexp-macro-assembler-mips.h is gone.

[Søren Thygesen Gjesse, 2010-01-29 13:06:22]

One thing I forgot. Please change the year to 2010 in the header for all the new
files.

[Alexandre, 2010-01-30 00:31:38]

Hi,

I worked on your comments.

I also updated to revision 3749. I removed fast-codegen.* and added full-codegen
files as I understood v8 was moving to this one.
Everything compiles and cctest work for me. I did not add code generation yet.

Regards,

Alexandre

http://codereview.chromium.org/543161/diff/3003/12018
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/3003/12018#newcode203
src/mips/assembler-mips-inl.h:203: ((Assembler::instr_at(pc_) &
kFunctionFieldMask) == JALR));
Fixed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> You need a one character indent of the last line of the expression.

http://codereview.chromium.org/543161/diff/3003/12038
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12038#newcode998
src/mips/assembler-mips.cc:998: cc << 8 | 3 << 4 | cond;
Split after =.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> 4 character indent here. Perhaps if splitting just after the = the rest can be
> on one line.

http://codereview.chromium.org/543161/diff/3003/12038#newcode1162
src/mips/assembler-mips.cc:1162: // 
Memory::Address_at(target_address_address_at(pc)) = target;
Fixed this section of comments. Also added undef statements.
Don't worry I can see the end of style fixes, and with that the code will be
much cleaner, which is good!
On 2010/01/29 10:47:11, antonm wrote:
> sorry for nit picking, once again comments at the start of the line.  Here and
> below.

http://codereview.chromium.org/543161/diff/3003/12040
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12040#newcode35
src/mips/constants-mips.h:35: #define UNIMPLEMENTED_()                          
                        \
Renamed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> I suggest naming this macro UNIMPLEMENTED_MIPS

http://codereview.chromium.org/543161/diff/3003/12040#newcode38
src/mips/constants-mips.h:38: #define UNSUPPORTED_()
v8::internal::PrintF("Unsupported instruction.\n")
Renamed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> and UNSUPPORTED_MIPS

http://codereview.chromium.org/543161/diff/3003/12040#newcode499
src/mips/constants-mips.h:499: // Instructions are read of out a code stream.
The only way to get a
Renamed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Rename isXXX -> IsXXX

http://codereview.chromium.org/543161/diff/3003/12028
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12028#newcode125
src/mips/disasm-mips.cc:125: // Each of these functions decodes one particular
instruction type.
Renamed like in the simulator.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> How about using names instead of numbers?

http://codereview.chromium.org/543161/diff/3003/12028#newcode164
src/mips/disasm-mips.cc:164: 
Fixed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Two empty lines between methods.

http://codereview.chromium.org/543161/diff/3003/12028#newcode235
src/mips/disasm-mips.cc:235: out_buffer_pos_ += v8i::OS::SNPrintF(out_buffer_ +
out_buffer_pos_,
Used 4 space indent.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Either aling with ( or use 4 space indent. If splitting after += the rest
might
> fit on one line.

http://codereview.chromium.org/543161/diff/3003/12028#newcode246
src/mips/disasm-mips.cc:246: out_buffer_pos_ += v8i::OS::SNPrintF(out_buffer_ +
out_buffer_pos_,
Fixed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/3003/12019
File src/mips/jump-target-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12019#newcode50
src/mips/jump-target-mips.cc:50: void JumpTarget::DoBranch(Condition cc, Hint
ignored,
Removed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Remove the two Register parameters for code to compile.

http://codereview.chromium.org/543161/diff/3003/12026
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12026#newcode256
src/mips/macro-assembler-mips.h:256: //
---------------------------------------------------------------------------
A lot has been removed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Please remove all the which are specific to V8 code generation and add them on
a
> needed basis.

http://codereview.chromium.org/543161/diff/3003/12026#newcode490
src/mips/macro-assembler-mips.h:490: class CodePatcher {
Removed already with the macro-assembler cleaning.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Just remove the code patcher for now and add it back when needed.

http://codereview.chromium.org/543161/diff/3003/12035
File src/mips/regexp-macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12035#newcode1
src/mips/regexp-macro-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Removed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Please remove this file as it is no longer needed.

http://codereview.chromium.org/543161/diff/3003/12022
File src/mips/regexp-macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12022#newcode1
src/mips/regexp-macro-assembler-mips.h:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Removed and modified test-regexp.cc accordingly.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Please remove this file as it is no longer needed.

http://codereview.chromium.org/543161/diff/3003/12041
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12041#newcode1231
src/mips/simulator-mips.cc:1231: //        case TGEI:
Removed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Code in comment.

http://codereview.chromium.org/543161/diff/3003/12041#newcode1252
src/mips/simulator-mips.cc:1252: //      case TGEI:
Removed.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Code in comment.

http://codereview.chromium.org/543161/diff/3003/12044
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/3003/12044#newcode208
src/mips/simulator-mips.h:208: // Executing is handled based on the instruction
type.
Renamed with Register, Immediate, and Jump suffixes. Also replaced Imm26 with
Jump in the assembler as you suggested before.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> How about using names instead of numbers here?

http://codereview.chromium.org/543161/diff/3003/12001
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/3003/12001#newcode180
test/cctest/test-assembler-mips.cc:180: //  __ mfhi(v1);
I forgot to remove this before committing. This file has been updated and there
is no more commented code here. (Except for example instructions which should
overflow)
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> Code in comments, and below.

http://codereview.chromium.org/543161/diff/3003/12049
File test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/543161/diff/3003/12049#newcode1
test/cctest/test-regexp.cc:1: // Copyright 2008 the V8 project authors. All
rights reserved.
Removed the mips include. However we need the typedef below.
On 2010/01/29 12:58:07, Søren Gjesse wrote:
> You can revert this file when src/mips/regexp-macro-assembler-mips.h is gone.

[Alexandre, 2010-01-30 02:41:05]

Hi,

Just a little fix as I found a bug in debug mode.

Alexandre

http://codereview.chromium.org/543161/diff/9055/9091
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9091#newcode462
src/mips/constants-mips.h:462: ASSERT(instrType() == kRegisterType ||
instrType() == kImmediateType);
This ASSERT causes an infinite loop in debug mode as it calls instrType() and
FunctionFieldRaw is also called by instrType().
I remove it for now.

[antonm, 2010-02-01 11:28:02]

LGTM, but please, wait for Søren's blessing.

http://codereview.chromium.org/543161/diff/9055/9089
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9089#newcode998
src/mips/assembler-mips.cc:998: COP1 | fmt | ft.code() << 16 | fs.code() <<
kFsShift | cc << 8 | 3 << 4 | cond;
doesn't that exceed 80 char limit?  overall, ideally ./tools/presubmit.py should
be run to see style violations.

http://codereview.chromium.org/543161/diff/9055/9083
File src/mips/cpu-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9083#newcode51
src/mips/cpu-mips.cc:51: // See
http://www.linux-mips.org/wiki/Cacheflush_Syscall
comment indentation, please.

[Søren Thygesen Gjesse, 2010-02-01 22:31:23]

Yet another round of comments. When they have been addressed there should not be
much more to do before this can be committed.

http://codereview.chromium.org/543161/diff/9055/9072
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/9055/9072#newcode76
src/mips/assembler-mips-inl.h:76: Operand::Operand(Object** opp) {
I don't think this one is needed. It is there for ARM, but is not used and
should probably be removed.

http://codereview.chromium.org/543161/diff/9055/9072#newcode82
src/mips/assembler-mips-inl.h:82: Operand::Operand(Context** cpp) {
I don't think this one is needed. It is there for ARM, but is not used and
should probably be removed.

http://codereview.chromium.org/543161/diff/9055/9089
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9089#newcode1138
src/mips/assembler-mips.cc:1138: ((instr1 == 0) && ((instr2 & kOpcodeMask) ==
ADDI ||
Please add a constant for no operation.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1142
src/mips/assembler-mips.cc:1142: if (instr1 == 0) {
Ditto.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1144
src/mips/assembler-mips.cc:1144: return (Address)(((instr2 & kImm16Mask) <<
16)>>16);
Change to C++ style cast. 4 concurrences below.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1166
src/mips/assembler-mips.cc:1166: // instr1 would be unused.
Using macros here is a bit obscure. We have the USE macro in globals.h to help
here. Alternatively use a

  Instr instr2 = ...
#ifdef DEBUG
  Instr instr1 = ...
  CHECK(...)
#endif

We normally change ASSERT to CHECK when inside an #ifdef DEBUG/#endif, but that
is not that important.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1179
src/mips/assembler-mips.cc:1179: uint32_t itarget = (uint32_t)(target);
C style case.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1184
src/mips/assembler-mips.cc:1184: *p       = 0x0;
Please add a constant for no operation.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1189
src/mips/assembler-mips.cc:1189: *p       = 0x0;
Ditto.

http://codereview.chromium.org/543161/diff/9055/9093
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9093#newcode367
src/mips/assembler-mips.h:367: #define DEFINE_INSTRUCTION_R_(instr)             
                             \
Looks as if this macro is not used any more.

http://codereview.chromium.org/543161/diff/9055/9093#newcode374
src/mips/assembler-mips.h:374: #define DEFINE_INSTRUCTION_I_(instr)             
                             \
Ditto.

http://codereview.chromium.org/543161/diff/9055/9093#newcode676
src/mips/assembler-mips.h:676: #undef DEFINE_INSTRUCTION_R_
Looks as if this macro is not used any more.

http://codereview.chromium.org/543161/diff/9055/9093#newcode677
src/mips/assembler-mips.h:677: #undef DEFINE_INSTRUCTION_I_
Ditto.

http://codereview.chromium.org/543161/diff/9055/9075
File src/mips/codegen-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9075#newcode188
src/mips/codegen-mips.cc:188: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9075#newcode224
src/mips/codegen-mips.cc:224: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9075#newcode274
src/mips/codegen-mips.cc:274: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9075#newcode289
src/mips/codegen-mips.cc:289: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9091
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9091#newcode34
src/mips/constants-mips.h:34: #ifdef DEBUG
Maybe define these macros to print in release mode as well.

http://codereview.chromium.org/543161/diff/9055/9091#newcode123
src/mips/constants-mips.h:123: typedef unsigned char byte_;
Why the trailing underscore? Is there a namespace problem? I can see in
constants-arm.h we have the same without the trailing underscore. Will it be
possible to reuse the byte typedef in globals.h?

http://codereview.chromium.org/543161/diff/9055/9091#newcode383
src/mips/constants-mips.h:383: enum IType {
Why not just Type? Is that not allowed?

http://codereview.chromium.org/543161/diff/9055/9091#newcode391
src/mips/constants-mips.h:391: IType instrType() const;
instrType -> InstructionType or just Type (which might conflict with the enum
above).

http://codereview.chromium.org/543161/diff/9055/9091#newcode462
src/mips/constants-mips.h:462: ASSERT(instrType() == kRegisterType ||
instrType() == kImmediateType);
On 2010/01/30 02:41:05, alexandre.rames wrote:
> This ASSERT causes an infinite loop in debug mode as it calls instrType() and
> FunctionFieldRaw is also called by instrType().
> I remove it for now.

Thats OK.

http://codereview.chromium.org/543161/diff/9055/9081
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9081#newcode256
src/mips/disasm-mips.cc:256: v8i::OS::SNPrintF(out_buffer_ + out_buffer_pos_,
"0x%05x", code);
4 space indent.

http://codereview.chromium.org/543161/diff/9055/9081#newcode267
src/mips/disasm-mips.cc:267: v8i::OS::SNPrintF(out_buffer_ + out_buffer_pos_,
"0x%03x", code);
Ditto.

http://codereview.chromium.org/543161/diff/9055/9081#newcode543
src/mips/disasm-mips.cc:543: break;
You don't need 'break' in default. Also some places below.

http://codereview.chromium.org/543161/diff/9055/9080
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9080#newcode41
src/mips/frames-mips.h:41: static const RegList kJSCallerSaved =
Is it possible to use the Register constants here, e.g. a0.bit() instead of 4.

http://codereview.chromium.org/543161/diff/9055/9080#newcode57
src/mips/frames-mips.h:57: // Saved temporaries
Ditto.

http://codereview.chromium.org/543161/diff/9055/9088
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9088#newcode122
src/mips/macro-assembler-mips.cc:122: // Will clobber 4 registers: object,
offset, scratch, ip.  The
You probably want to remove this comment as it will be difficult to know how
this will be implemented.

http://codereview.chromium.org/543161/diff/9055/9079
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9079#newcode208
src/mips/macro-assembler-mips.h:208: void MultiPush_reversed(RegList regs);
MultiPush_reversed -> MultiPushReversed

http://codereview.chromium.org/543161/diff/9055/9079#newcode226
src/mips/macro-assembler-mips.h:226: void MultiPop_reversed(RegList regs);
MultiPop_reversed -> MultiPopReversed

http://codereview.chromium.org/543161/diff/9055/9079#newcode253
src/mips/macro-assembler-mips.h:253: Register scratch = at) {
Add ASSERT_EQ(0, kSmiTag).

http://codereview.chromium.org/543161/diff/9055/9079#newcode260
src/mips/macro-assembler-mips.h:260: Register scratch = at) {
Ditto.

http://codereview.chromium.org/543161/diff/9055/9092
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9092#newcode53
src/mips/simulator-mips.cc:53: bool haveSameSign(int32_t a, int32_t b) {
Uppercase H.

http://codereview.chromium.org/543161/diff/9055/9092#newcode244
src/mips/simulator-mips.cc:244: 
Please use static const int kXxxYyy for these constants.

http://codereview.chromium.org/543161/diff/9055/9092#newcode601
src/mips/simulator-mips.cc:601: ASSERT((fpureg >= 0) && (fpureg <
kNumFPURegisters) && (fpureg%2 == 0));
Is fpureg%2 == 0 in the ASSERT correct when just setting an 32-bit value?

http://codereview.chromium.org/543161/diff/9055/9092#newcode606
src/mips/simulator-mips.cc:606: ASSERT((fpureg >= 0) && (fpureg <
kNumFPURegisters) && (fpureg%2 == 0));
Please format (fpureg%2 == 0) as ((fpureg % 2) == 0)

http://codereview.chromium.org/543161/diff/9055/9092#newcode607
src/mips/simulator-mips.cc:607:
*reinterpret_cast<double*>(&FPUregisters_[fpureg]) = value;
Would a bit_cast be better here?

http://codereview.chromium.org/543161/diff/9055/9092#newcode653
src/mips/simulator-mips.cc:653: if ((addr & 3) == 0) {
Please consider using kPointerAlignmentMask from globals.h instead of the
constant 3 (three times more below).

http://codereview.chromium.org/543161/diff/9055/9092#newcode675
src/mips/simulator-mips.cc:675: if ((addr & 3) == 0) {
Should this be 7 instead of 3? If so please add constants kDoubleAlignmentBits,
kDoubleAlignment and kDoubleAlignmentMask.

http://codereview.chromium.org/543161/diff/9055/9092#newcode686
src/mips/simulator-mips.cc:686: if ((addr & 3) == 0) {
Ditto.

http://codereview.chromium.org/543161/diff/9055/9092#newcode863
src/mips/simulator-mips.cc:863: // Instruction fields
Please fix the alignment of the following code - or get rid of it altogether.

http://codereview.chromium.org/543161/diff/9055/9092#newcode912
src/mips/simulator-mips.cc:912: UNIMPLEMENTED_MIPS();
No break on the default case (more below).

http://codereview.chromium.org/543161/diff/9055/9092#newcode1122
src/mips/simulator-mips.cc:1122: set_register(LO, rs/rt);
Spacing: rs / rt (three more just below).

http://codereview.chromium.org/543161/diff/9055/9092#newcode1299
src/mips/simulator-mips.cc:1299: alu_out = (oe_imm16<<16);
Spaces around << (several other places).

http://codereview.chromium.org/543161/diff/9055/9092#newcode1616
src/mips/simulator-mips.cc:1616: 
Please add another blank line.

http://codereview.chromium.org/543161/diff/9055/9095
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9095#newcode205
src/mips/simulator-mips.h:205: 
Does these halfword read/writes make any sense on a MIPS platform? If not please
remove them.

http://codereview.chromium.org/543161/diff/9055/9095#newcode220
src/mips/simulator-mips.h:220: inline int32_t getDoubleHIW(double* addr);
get -> Get

http://codereview.chromium.org/543161/diff/9055/9095#newcode221
src/mips/simulator-mips.h:221: inline int32_t getDoubleLOW(double* addr);
Ditto.

http://codereview.chromium.org/543161/diff/9055/9095#newcode223
src/mips/simulator-mips.h:223: inline int32_t setDoubleHIW(double* addr);
set -> Set

http://codereview.chromium.org/543161/diff/9055/9095#newcode224
src/mips/simulator-mips.h:224: inline int32_t setDoubleLOW(double* addr);
Ditto.

http://codereview.chromium.org/543161/diff/9055/9095#newcode247
src/mips/simulator-mips.h:247: enum Exception {
Please use either kXxxYxx or XXX_YYY naming, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer....

http://codereview.chromium.org/543161/diff/9055/9090
File src/mips/stub-cache-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9090#newcode1
src/mips/stub-cache-mips.cc:1: // Copyright 2006-2010 the V8 project authors.
All rights reserved.
Don't you want all the UNIMPLEMENTED in this file to be UNIMPLEMENTED_MIPS?

http://codereview.chromium.org/543161/diff/9055/9078
File src/mips/virtual-frame-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9078#newcode1
src/mips/virtual-frame-mips.cc:1: // Copyright 2006-2010 the V8 project authors.
All rights reserved.
Don't you want all the UNIMPLEMENTED in this file to be UNIMPLEMENTED_MIPS?

http://codereview.chromium.org/543161/diff/9055/9068
File src/objects-inl.h (right):

http://codereview.chromium.org/543161/diff/9055/9068#newcode1032
src/objects-inl.h:1032: // WRITE_FIELD does not update the remembered set, but
there is no need here.
Please revert this.

http://codereview.chromium.org/543161/diff/9055/9062
File src/objects.h (right):

http://codereview.chromium.org/543161/diff/9055/9062#newcode1105
src/objects.h:1105: 
Please revert this.

http://codereview.chromium.org/543161/diff/9055/9066
File src/platform-linux.cc (right):

http://codereview.chromium.org/543161/diff/9055/9066#newcode750
src/platform-linux.cc:750: // Implement this on MIPS.
Please UNIMPLELEMTED() here.

http://codereview.chromium.org/543161/diff/9055/9056
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9056#newcode1
test/cctest/test-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
2006-2008 -> 2010.

http://codereview.chromium.org/543161/diff/9055/9056#newcode79
test/cctest/test-assembler-mips.cc:79: __ sllv(zero_reg, zero_reg, zero_reg);  
// nop
Perhaps add Nop to the macro assembler?

http://codereview.chromium.org/543161/diff/9055/9056#newcode183
test/cctest/test-assembler-mips.cc:183: __ nop();
Code in comments.

[Alexandre, 2010-02-02 02:51:35]

Hi,

I have addressed your comments.

Regards,

Alexandre

http://codereview.chromium.org/543161/diff/9055/9072
File src/mips/assembler-mips-inl.h (right):

http://codereview.chromium.org/543161/diff/9055/9072#newcode76
src/mips/assembler-mips-inl.h:76: Operand::Operand(Object** opp) {
Removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> I don't think this one is needed. It is there for ARM, but is not used and
> should probably be removed.

http://codereview.chromium.org/543161/diff/9055/9072#newcode82
src/mips/assembler-mips-inl.h:82: Operand::Operand(Context** cpp) {
Removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> I don't think this one is needed. It is there for ARM, but is not used and
> should probably be removed.

http://codereview.chromium.org/543161/diff/9055/9089
File src/mips/assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9089#newcode998
src/mips/assembler-mips.cc:998: COP1 | fmt | ft.code() << 16 | fs.code() <<
kFsShift | cc << 8 | 3 << 4 | cond;
I am sorry, it does. Moved part of the line after the assignment.
On 2010/02/01 11:28:02, antonm wrote:
> doesn't that exceed 80 char limit?  overall, ideally ./tools/presubmit.py
should
> be run to see style violations.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1138
src/mips/assembler-mips.cc:1138: ((instr1 == 0) && ((instr2 & kOpcodeMask) ==
ADDI ||
dded a nopInstr constant in constants-mips.h, and use it here.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please add a constant for no operation.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1142
src/mips/assembler-mips.cc:1142: if (instr1 == 0) {
Added a nopInstr constant in constants-mips.h, and use it here.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1144
src/mips/assembler-mips.cc:1144: return (Address)(((instr2 & kImm16Mask) <<
16)>>16);
Corrected with a reinterpret_cast.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Change to C++ style cast. 4 concurrences below.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1166
src/mips/assembler-mips.cc:1166: // instr1 would be unused.
The ifdef DEBUG is more understandable. Changed to use your suggested syntax.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Using macros here is a bit obscure. We have the USE macro in globals.h to help
> here. Alternatively use a
> 
>   Instr instr2 = ...
> #ifdef DEBUG
>   Instr instr1 = ...
>   CHECK(...)
> #endif
> 
> We normally change ASSERT to CHECK when inside an #ifdef DEBUG/#endif, but
that
> is not that important.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1179
src/mips/assembler-mips.cc:1179: uint32_t itarget = (uint32_t)(target);
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> C style case.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1184
src/mips/assembler-mips.cc:1184: *p       = 0x0;
Use the new nopInstr constant.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please add a constant for no operation.

http://codereview.chromium.org/543161/diff/9055/9089#newcode1189
src/mips/assembler-mips.cc:1189: *p       = 0x0;
Use the new nopInstr constant.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9093
File src/mips/assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9093#newcode367
src/mips/assembler-mips.h:367: #define DEFINE_INSTRUCTION_R_(instr)             
                             \
It is indeed obsolete. Removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Looks as if this macro is not used any more.

http://codereview.chromium.org/543161/diff/9055/9093#newcode374
src/mips/assembler-mips.h:374: #define DEFINE_INSTRUCTION_I_(instr)             
                             \
Also removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9093#newcode676
src/mips/assembler-mips.h:676: #undef DEFINE_INSTRUCTION_R_
Removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Looks as if this macro is not used any more.

http://codereview.chromium.org/543161/diff/9055/9093#newcode677
src/mips/assembler-mips.h:677: #undef DEFINE_INSTRUCTION_I_
Removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9075
File src/mips/codegen-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9075#newcode188
src/mips/codegen-mips.cc:188: UNIMPLEMENTED();
Forgot to run the search/replace for UNIMPLEMENTED without "_". Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9075#newcode224
src/mips/codegen-mips.cc:224: UNIMPLEMENTED();
Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9075#newcode274
src/mips/codegen-mips.cc:274: UNIMPLEMENTED();
Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9075#newcode289
src/mips/codegen-mips.cc:289: UNIMPLEMENTED();
Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> UNIMPLEMENTED_MIPS()

http://codereview.chromium.org/543161/diff/9055/9091
File src/mips/constants-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9091#newcode34
src/mips/constants-mips.h:34: #ifdef DEBUG
Why not. I did so to keep the release version free of unimplemented flooding,
but it can always be removed easily if needed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Maybe define these macros to print in release mode as well.

http://codereview.chromium.org/543161/diff/9055/9091#newcode123
src/mips/constants-mips.h:123: typedef unsigned char byte_;
Without the underscore this typedef conflicted with the one in globals.h when
compiling test-regexp.cc .
"test/cctest/test-regexp.cc:1274: error: reference to 'byte' is ambiguous".
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Why the trailing underscore? Is there a namespace problem? I can see in
> constants-arm.h we have the same without the trailing underscore. Will it be
> possible to reuse the byte typedef in globals.h?

http://codereview.chromium.org/543161/diff/9055/9091#newcode383
src/mips/constants-mips.h:383: enum IType {
No special reason, that was the first name I thought of! I change it to Type.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Why not just Type? Is that not allowed?

http://codereview.chromium.org/543161/diff/9055/9091#newcode391
src/mips/constants-mips.h:391: IType instrType() const;
Changed the name to InstructionType.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> instrType -> InstructionType or just Type (which might conflict with the enum
> above).

http://codereview.chromium.org/543161/diff/9055/9083
File src/mips/cpu-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9083#newcode51
src/mips/cpu-mips.cc:51: // See
http://www.linux-mips.org/wiki/Cacheflush_Syscall
Fixed.
On 2010/02/01 11:28:02, antonm wrote:
> comment indentation, please.

http://codereview.chromium.org/543161/diff/9055/9081
File src/mips/disasm-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9081#newcode256
src/mips/disasm-mips.cc:256: v8i::OS::SNPrintF(out_buffer_ + out_buffer_pos_,
"0x%05x", code);
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> 4 space indent.

http://codereview.chromium.org/543161/diff/9055/9081#newcode267
src/mips/disasm-mips.cc:267: v8i::OS::SNPrintF(out_buffer_ + out_buffer_pos_,
"0x%03x", code);
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9081#newcode543
src/mips/disasm-mips.cc:543: break;
Removed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> You don't need 'break' in default. Also some places below.

http://codereview.chromium.org/543161/diff/9055/9080
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9080#newcode41
src/mips/frames-mips.h:41: static const RegList kJSCallerSaved =
The Register structure is declared in assembler-mips.h, and registers in
assembler-mips.cc, so it's not very convenient. 
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Is it possible to use the Register constants here, e.g. a0.bit() instead of 4.

http://codereview.chromium.org/543161/diff/9055/9080#newcode57
src/mips/frames-mips.h:57: // Saved temporaries
See previous comment.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9088
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9088#newcode122
src/mips/macro-assembler-mips.cc:122: // Will clobber 4 registers: object,
offset, scratch, ip.  The
Indeed! I fear there might be other like these. I'll remove them when I meet
some.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> You probably want to remove this comment as it will be difficult to know how
> this will be implemented.

http://codereview.chromium.org/543161/diff/9055/9079
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9079#newcode208
src/mips/macro-assembler-mips.h:208: void MultiPush_reversed(RegList regs);
Sorry about that. Changed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> MultiPush_reversed -> MultiPushReversed

http://codereview.chromium.org/543161/diff/9055/9079#newcode226
src/mips/macro-assembler-mips.h:226: void MultiPop_reversed(RegList regs);
Changed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> MultiPop_reversed -> MultiPopReversed

http://codereview.chromium.org/543161/diff/9055/9079#newcode253
src/mips/macro-assembler-mips.h:253: Register scratch = at) {
Added.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Add ASSERT_EQ(0, kSmiTag).

http://codereview.chromium.org/543161/diff/9055/9079#newcode260
src/mips/macro-assembler-mips.h:260: Register scratch = at) {
Added.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9092
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9092#newcode53
src/mips/simulator-mips.cc:53: bool haveSameSign(int32_t a, int32_t b) {
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Uppercase H.

http://codereview.chromium.org/543161/diff/9055/9092#newcode244
src/mips/simulator-mips.cc:244: 
These are used below for formatting purpose, that's why #define statements are
used. It is quite convenient. However we could still change it to static const
int and add some other defines or format manually below.
(This code is nearly copied line for line form ARM code.)
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please use static const int kXxxYyy for these constants.

http://codereview.chromium.org/543161/diff/9055/9092#newcode601
src/mips/simulator-mips.cc:601: ASSERT((fpureg >= 0) && (fpureg <
kNumFPURegisters) && (fpureg%2 == 0));
It is not. Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Is fpureg%2 == 0 in the ASSERT correct when just setting an 32-bit value?

http://codereview.chromium.org/543161/diff/9055/9092#newcode606
src/mips/simulator-mips.cc:606: ASSERT((fpureg >= 0) && (fpureg <
kNumFPURegisters) && (fpureg%2 == 0));
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please format (fpureg%2 == 0) as ((fpureg % 2) == 0)

http://codereview.chromium.org/543161/diff/9055/9092#newcode607
src/mips/simulator-mips.cc:607:
*reinterpret_cast<double*>(&FPUregisters_[fpureg]) = value;
Yes. This was completely wrong. I actually forgot to change FPUregisters_ type
to int32_t.
Fixed. Now we use bit_cast.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Would a bit_cast be better here?

http://codereview.chromium.org/543161/diff/9055/9092#newcode653
src/mips/simulator-mips.cc:653: if ((addr & 3) == 0) {
Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please consider using kPointerAlignmentMask from globals.h instead of the
> constant 3 (three times more below).

http://codereview.chromium.org/543161/diff/9055/9092#newcode675
src/mips/simulator-mips.cc:675: if ((addr & 3) == 0) {
It should. Added constants in constants-mips.h.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Should this be 7 instead of 3? If so please add constants
kDoubleAlignmentBits,
> kDoubleAlignment and kDoubleAlignmentMask.

http://codereview.chromium.org/543161/diff/9055/9092#newcode686
src/mips/simulator-mips.cc:686: if ((addr & 3) == 0) {
Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9092#newcode863
src/mips/simulator-mips.cc:863: // Instruction fields
Fixed the alignment. I know this is a lot of variable, but it makes the
following code more readable.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please fix the alignment of the following code - or get rid of it altogether.

http://codereview.chromium.org/543161/diff/9055/9092#newcode912
src/mips/simulator-mips.cc:912: UNIMPLEMENTED_MIPS();
Removed break statements.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> No break on the default case (more below).

http://codereview.chromium.org/543161/diff/9055/9092#newcode1122
src/mips/simulator-mips.cc:1122: set_register(LO, rs/rt);
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Spacing: rs / rt (three more just below).

http://codereview.chromium.org/543161/diff/9055/9092#newcode1299
src/mips/simulator-mips.cc:1299: alu_out = (oe_imm16<<16);
Fixed (for ">>" also).
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Spaces around << (several other places).

http://codereview.chromium.org/543161/diff/9055/9092#newcode1616
src/mips/simulator-mips.cc:1616: 
Added.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please add another blank line.

http://codereview.chromium.org/543161/diff/9055/9095
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9095#newcode205
src/mips/simulator-mips.h:205: 
Yes it does, although I have not implemented half-word load and store
instructions yet. (lh, lhu, sh)
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Does these halfword read/writes make any sense on a MIPS platform? If not
please
> remove them.

http://codereview.chromium.org/543161/diff/9055/9095#newcode220
src/mips/simulator-mips.h:220: inline int32_t getDoubleHIW(double* addr);
Changed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> get -> Get

http://codereview.chromium.org/543161/diff/9055/9095#newcode221
src/mips/simulator-mips.h:221: inline int32_t getDoubleLOW(double* addr);
Changed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9095#newcode223
src/mips/simulator-mips.h:223: inline int32_t setDoubleHIW(double* addr);
Changed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> set -> Set

http://codereview.chromium.org/543161/diff/9055/9095#newcode224
src/mips/simulator-mips.h:224: inline int32_t setDoubleLOW(double* addr);
Changed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/543161/diff/9055/9095#newcode247
src/mips/simulator-mips.h:247: enum Exception {
Renamed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please use either kXxxYxx or XXX_YYY naming, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer....

http://codereview.chromium.org/543161/diff/9055/9090
File src/mips/stub-cache-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9090#newcode1
src/mips/stub-cache-mips.cc:1: // Copyright 2006-2010 the V8 project authors.
All rights reserved.
I used a script to transform all "UNIPLEMENTED_" to "UNIMPLEMENTED_MIPS" but
forgot the statements without underscore. Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Don't you want all the UNIMPLEMENTED in this file to be UNIMPLEMENTED_MIPS?

http://codereview.chromium.org/543161/diff/9055/9078
File src/mips/virtual-frame-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9078#newcode1
src/mips/virtual-frame-mips.cc:1: // Copyright 2006-2010 the V8 project authors.
All rights reserved.
Fixed.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Don't you want all the UNIMPLEMENTED in this file to be UNIMPLEMENTED_MIPS?

http://codereview.chromium.org/543161/diff/9055/9068
File src/objects-inl.h (right):

http://codereview.chromium.org/543161/diff/9055/9068#newcode1032
src/objects-inl.h:1032: // WRITE_FIELD does not update the remembered set, but
there is no need here.
Reverted.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please revert this.

http://codereview.chromium.org/543161/diff/9055/9062
File src/objects.h (right):

http://codereview.chromium.org/543161/diff/9055/9062#newcode1105
src/objects.h:1105: 
Reverted.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please revert this.

http://codereview.chromium.org/543161/diff/9055/9066
File src/platform-linux.cc (right):

http://codereview.chromium.org/543161/diff/9055/9066#newcode750
src/platform-linux.cc:750: // Implement this on MIPS.
Added.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Please UNIMPLELEMTED() here.

http://codereview.chromium.org/543161/diff/9055/9056
File test/cctest/test-assembler-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9056#newcode1
test/cctest/test-assembler-mips.cc:1: // Copyright 2006-2008 the V8 project
authors. All rights reserved.
Corrected.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> 2006-2008 -> 2010.

http://codereview.chromium.org/543161/diff/9055/9056#newcode79
test/cctest/test-assembler-mips.cc:79: __ sllv(zero_reg, zero_reg, zero_reg);  
// nop
It already exists. I use it below for other tests, but since it was the only
instruction here requiring the MacroAssembler I kept the Assembler.
Changed the Assembler to a MacroAssembler in this test.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Perhaps add Nop to the macro assembler?

http://codereview.chromium.org/543161/diff/9055/9056#newcode183
test/cctest/test-assembler-mips.cc:183: __ nop();
I though we could let this here as it can be used to check exceptions work. This
was intended as something to illustrate exceptions.
I remove it if you think it should anyway not be there.
On 2010/02/01 22:31:23, Søren Gjesse wrote:
> Code in comments.

[Alexandre, 2010-02-04 02:30:39]

http://codereview.chromium.org/543161/diff/13005/13028
File src/mips/macro-assembler-mips.h (right):

http://codereview.chromium.org/543161/diff/13005/13028#newcode127
src/mips/macro-assembler-mips.h:127: // Sets the remembered set bit for
[address+offset], where address is the
Removed this comment.

[Søren Thygesen Gjesse, 2010-02-04 12:11:36]

LGTM

Thank you for this patch!

I will try to land it in the repository, see
http://codereview.chromium.org/561072.

http://codereview.chromium.org/543161/diff/9055/9080
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9080#newcode41
src/mips/frames-mips.h:41: static const RegList kJSCallerSaved =
On 2010/02/02 02:51:35, alexandre.rames wrote:
> The Register structure is declared in assembler-mips.h, and registers in
> assembler-mips.cc, so it's not very convenient. 
> On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > Is it possible to use the Register constants here, e.g. a0.bit() instead of
4.
> 
> 

OK

http://codereview.chromium.org/543161/diff/9055/9080#newcode57
src/mips/frames-mips.h:57: // Saved temporaries
On 2010/02/02 02:51:35, alexandre.rames wrote:
> See previous comment.
> On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > Ditto.
> 
> 

OK

http://codereview.chromium.org/543161/diff/9055/9092
File src/mips/simulator-mips.cc (right):

http://codereview.chromium.org/543161/diff/9055/9092#newcode244
src/mips/simulator-mips.cc:244: 
On 2010/02/02 02:51:35, alexandre.rames wrote:
> These are used below for formatting purpose, that's why #define statements are
> used. It is quite convenient. However we could still change it to static const
> int and add some other defines or format manually below.
> (This code is nearly copied line for line form ARM code.)
> On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > Please use static const int kXxxYyy for these constants.
> 
> 

OK

http://codereview.chromium.org/543161/diff/9055/9095
File src/mips/simulator-mips.h (right):

http://codereview.chromium.org/543161/diff/9055/9095#newcode205
src/mips/simulator-mips.h:205: 
On 2010/02/02 02:51:35, alexandre.rames wrote:
> Yes it does, although I have not implemented half-word load and store
> instructions yet. (lh, lhu, sh)
> On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > Does these halfword read/writes make any sense on a MIPS platform? If not
> please
> > remove them.
> 
> 

Fine, we will keep them there.

[Søren Thygesen Gjesse, 2010-02-04 20:46:31]

This has been landed as r3799, see http://codereview.chromium.org/561072.

Closing this issue.

On 2010/02/04 12:11:36, Søren Gjesse wrote:
> LGTM
> 
> Thank you for this patch!
> 
> I will try to land it in the repository, see
> http://codereview.chromium.org/561072.
> 
> http://codereview.chromium.org/543161/diff/9055/9080
> File src/mips/frames-mips.h (right):
> 
> http://codereview.chromium.org/543161/diff/9055/9080#newcode41
> src/mips/frames-mips.h:41: static const RegList kJSCallerSaved =
> On 2010/02/02 02:51:35, alexandre.rames wrote:
> > The Register structure is declared in assembler-mips.h, and registers in
> > assembler-mips.cc, so it's not very convenient. 
> > On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > > Is it possible to use the Register constants here, e.g. a0.bit() instead
of
> 4.
> > 
> > 
> 
> OK
> 
> http://codereview.chromium.org/543161/diff/9055/9080#newcode57
> src/mips/frames-mips.h:57: // Saved temporaries
> On 2010/02/02 02:51:35, alexandre.rames wrote:
> > See previous comment.
> > On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > > Ditto.
> > 
> > 
> 
> OK
> 
> http://codereview.chromium.org/543161/diff/9055/9092
> File src/mips/simulator-mips.cc (right):
> 
> http://codereview.chromium.org/543161/diff/9055/9092#newcode244
> src/mips/simulator-mips.cc:244: 
> On 2010/02/02 02:51:35, alexandre.rames wrote:
> > These are used below for formatting purpose, that's why #define statements
are
> > used. It is quite convenient. However we could still change it to static
const
> > int and add some other defines or format manually below.
> > (This code is nearly copied line for line form ARM code.)
> > On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > > Please use static const int kXxxYyy for these constants.
> > 
> > 
> 
> OK
> 
> http://codereview.chromium.org/543161/diff/9055/9095
> File src/mips/simulator-mips.h (right):
> 
> http://codereview.chromium.org/543161/diff/9055/9095#newcode205
> src/mips/simulator-mips.h:205: 
> On 2010/02/02 02:51:35, alexandre.rames wrote:
> > Yes it does, although I have not implemented half-word load and store
> > instructions yet. (lh, lhu, sh)
> > On 2010/02/01 22:31:23, Søren Gjesse wrote:
> > > Does these halfword read/writes make any sense on a MIPS platform? If not
> > please
> > > remove them.
> > 
> > 
> 
> Fine, we will keep them there.