Support for MIPS in architecture independent files....

Support for MIPS in architecture independent files.
Code for MIPS assembly generation, simulator, and disassembler.
Not all instructions supported in the simulator have been thoroughly tested yet.

[Søren Thygesen Gjesse, 2010-01-19 22:59:12]

Some initial comments.

http://codereview.chromium.org/549079/diff/1/5
File src/allocation.cc (right):

http://codereview.chromium.org/549079/diff/1/5#newcode109
src/allocation.cc:109: void PreallocatedStorage::Init(size_t size) {
Please revert this.

http://codereview.chromium.org/549079/diff/1/22
File src/assembler.cc (right):

http://codereview.chromium.org/549079/diff/1/22#newcode55
src/assembler.cc:55: #elif V8_TARGET_ARCH_MIPS
I think we can safely ignore the native code RegExp implementation for now. Use
regexp=interpreted when building or change the SConstruct file to default regexp
to interpreted when building for mips.

http://codereview.chromium.org/549079/diff/1/22#newcode648
src/assembler.cc:648: #elif V8_TARGET_ARCH_MIPS
See above.

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

http://codereview.chromium.org/549079/diff/1/17#newcode512
src/assembler.h:512: static inline uint16_t NumBitsSet(uint32_t x) {
Rename NumBitsSet to NumberOfBitsSet.

How about just returning int?

http://codereview.chromium.org/549079/diff/1/17#newcode514
src/assembler.h:514: for(int i=0; i<32; i++) {
Please add spaces like this:

for (int i = 0; i < 32; i++) {

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo....

I don't know whether the performance of this function is important, but take a
look at
http://www-graphics.stanford.edu/~seander/bithacks.html#CountBitsSetNaive for
alternatives.

http://codereview.chromium.org/549079/diff/1/17#newcode515
src/assembler.h:515: if(x & 1<<i) num_bits_set++;
Add spaces here like this

if (x & (1 << i)) num_bits_set++;

http://codereview.chromium.org/549079/diff/1/64
File src/checks.h (left):

http://codereview.chromium.org/549079/diff/1/64#oldcode54
src/checks.h:54: 
Please revert this.

http://codereview.chromium.org/549079/diff/1/11
File src/codegen.h (right):

http://codereview.chromium.org/549079/diff/1/11#newcode153
src/codegen.h:153: #ifndef V8_TARGET_ARCH_MIPS
Having a platform specific #ifdef in a platform independent file should be
avoided.

http://codereview.chromium.org/549079/diff/1/11#newcode156
src/codegen.h:156: inline void Branch(Condition cc, Register src1 = zero_reg,
Look at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
forformatting of parameter list.

http://codereview.chromium.org/549079/diff/1/12
File src/compiler.cc (right):

http://codereview.chromium.org/549079/diff/1/12#newcode123
src/compiler.cc:123: #ifndef V8_TARGET_ARCH_MIPS
Please get rid of this #ifdef. You can create a FastCodeGenerator for MIPS which
does bailout in all cases.

http://codereview.chromium.org/549079/diff/1/26
File src/disasm.h (right):

http://codereview.chromium.org/549079/diff/1/26#newcode42
src/disasm.h:42: #ifdef V8_TARGET_ARCH_MIPS
This #ifdef should not be needed. Platform specific stuff has already spilled
into this file (NameOfXMMRegister). That is not good practice, but not your
fault.

http://codereview.chromium.org/549079/diff/1/8
File src/flag-definitions.h (right):

http://codereview.chromium.org/549079/diff/1/8#newcode208
src/flag-definitions.h:208: DEFINE_bool(trace_sim, true, "trace simulator
execution")
Please keep the default to false.

http://codereview.chromium.org/549079/diff/1/66
File src/globals.h (right):

http://codereview.chromium.org/549079/diff/1/66#newcode60
src/globals.h:60: #define V8_TARGET_CAN_READ_UNALIGNED 0
defining V8_TARGET_CAN_READ_UNALIGNED to 0 should not be needed.

http://codereview.chromium.org/549079/diff/1/66#newcode180
src/globals.h:180: // On mips 0xbaddead is the encoding of jump to 0xeb77ab4.
I don't think this is important. The handle zap value is placed where object
pointers used to be, and will most likely be used as an object value and not
executed code. Getting a segfault should be fine.

http://codereview.chromium.org/549079/diff/1/4
File src/heap.cc (right):

http://codereview.chromium.org/549079/diff/1/4#newcode47
src/heap.cc:47: #elif V8_TARGET_ARCH_MIPS && V8_NATIVE_REGEXP
I think you can safely ignore native RegExp for MIPS for now.

http://codereview.chromium.org/549079/diff/1/4#newcode1338
src/heap.cc:1338: 
Accidental edit?

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

http://codereview.chromium.org/549079/diff/1/63#newcode53
src/jsregexp.cc:53: #elif V8_TARGET_ARCH_MIPS
I think you can safely ignore native RegExp for MIPS for now.

http://codereview.chromium.org/549079/diff/1/63#newcode4481
src/jsregexp.cc:4481: #elif V8_TARGET_ARCH_MIPS
I think you can safely ignore native RegExp for MIPS for now.

http://codereview.chromium.org/549079/diff/1/23
File src/jump-target.h (right):

http://codereview.chromium.org/549079/diff/1/23#newcode116
src/jump-target.h:116: #ifndef V8_TARGET_ARCH_MIPS
Like in codegen.h having a platform specific #ifdef in a platform independent
file should be avoided.

http://codereview.chromium.org/549079/diff/1/23#newcode120
src/jump-target.h:120: virtual void Branch(Condition cc, Hint hint = no_hint,
Register src1 = zero_reg, const Operand& src2 = Operand(zero_reg));
Long line, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Line_L....

http://codereview.chromium.org/549079/diff/1/23#newcode161
src/jump-target.h:161: #ifndef V8_TARGET_ARCH_MIPS
See above.

http://codereview.chromium.org/549079/diff/1/23#newcode164
src/jump-target.h:164: void DoBranch(Condition cc, Hint hint, Register src1 =
zero_reg, const Operand& src2 = Operand(zero_reg));
See above.

http://codereview.chromium.org/549079/diff/1/23#newcode221
src/jump-target.h:221: // code after the branch.
See above.

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

http://codereview.chromium.org/549079/diff/1/51#newcode1
src/mips/assembler-mips.cc:1: // Copyright 2006-2008 the V8 project authors. All
rights reserved.
Please use the copyright header from src/mips/assembler-mips-inl.h mentioning
Sun Microsystems Inc. here as well.

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

http://codereview.chromium.org/549079/diff/1/58#newcode1
src/mips/assembler-mips.h:1: // Copyright 2006-2008 the V8 project authors. All
rights reserved.
Please use the copyright header from src/mips/assembler-mips-inl.h mentioning
Sun Microsystems Inc. here as well.

http://codereview.chromium.org/549079/diff/1/30
File src/mips/cfg-mips.cc (right):

http://codereview.chromium.org/549079/diff/1/30#newcode1
src/mips/cfg-mips.cc:1: //
You can safely remove this file as using a control flow graph was an experiment
which has been abandonned.

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

http://codereview.chromium.org/549079/diff/1/34#newcode1
src/mips/codegen-mips.cc:1: // Copyright 2006-2008 the V8 project authors. All
rights reserved.
Please remove all the code in comments.

http://codereview.chromium.org/549079/diff/1/49
File src/mips/codegen-mips.h (right):

http://codereview.chromium.org/549079/diff/1/49#newcode466
src/mips/codegen-mips.h:466: //    // Encode the parameters in a unique 16 bit
value.
Please remove code in comments.

http://codereview.chromium.org/549079/diff/1/49#newcode477
src/mips/codegen-mips.h:477: //    if (constant_rhs ==
CodeGenerator::kUnknownIntValue) return false;
Ditto.

http://codereview.chromium.org/549079/diff/1/49#newcode490
src/mips/codegen-mips.h:490: //    if (!specialized_on_rhs_) return 0;
Ditto.

http://codereview.chromium.org/549079/diff/1/49#newcode522
src/mips/codegen-mips.h:522: //    if (!specialized_on_rhs_) {
Ditto.

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

http://codereview.chromium.org/549079/diff/1/54#newcode13
src/mips/constants-mips.h:13: // (
http://web.asic.sdesigns.com/tools/libs/mips/doc/74K/doc/MIPS32-ISA.pdf )
This link seems not to work. How about using a www.mips.com URL? Even if it
requires login.

http://codereview.chromium.org/549079/diff/1/54#newcode112
src/mips/constants-mips.h:112: J         =   ( (0<<3) + 2 ) << opcode_o,
Formatting like ((0 << 3) + 2) << opcode_o matches the rest of the V8 code
better.

http://codereview.chromium.org/549079/diff/1/54#newcode128
src/mips/constants-mips.h:128: COP1      =   ( (2<<3) + 1 ) << opcode_o,   //
Coprocessor 1 class (floating point)
Long line.

http://codereview.chromium.org/549079/diff/1/54#newcode350
src/mips/constants-mips.h:350: return Bits(rs_o+rs_l-1, rs_o);
Format expression with spaces rs_o + rs_l - 1.

http://codereview.chromium.org/549079/diff/1/19
File src/platform-linux.cc (right):

http://codereview.chromium.org/549079/diff/1/19#newcode98
src/platform-linux.cc:98: #ifdef V8_TARGET_ARCH_MIPS
Combine with #else above to #elif.

[Søren Thygesen Gjesse, 2010-01-20 15:30:28]

With these changes to the SConstruct file the pseudo architecture mips-simu
should not be needed.

http://codereview.chromium.org/549079/diff/1/67
File SConstruct (right):

http://codereview.chromium.org/549079/diff/1/67#newcode176
SConstruct:176: 'arch:mips': {
If you change this section to

      'CPPDEFINES':   ['V8_TARGET_ARCH_MIPS'],
      'simulator:none': {
        'CCFLAGS':      ['-EL', '-mips32r2', '-Wa,-mips32r2', '-fno-inline'],
        'LDFLAGS':      ['-EL']
      }

The flags for the MIPS gcc will now only become active when not compiling for
the simulator.

http://codereview.chromium.org/549079/diff/1/67#newcode185
SConstruct:185: 'arch:mips-simu': {
With the change to arch:mips above the pseudo arch mips-simu is no longer
needed.

http://codereview.chromium.org/549079/diff/1/67#newcode300
SConstruct:300: 'arch:mips-simu': {
With the change to arch:mips above the pseudo arch mips-simu is no longer
needed.

http://codereview.chromium.org/549079/diff/1/67#newcode301
SConstruct:301: 'CPPDEFINES':   ['V8_TARGET_ARCH_MIPS', 'NO_NATIVES',
'MIPS_SIMU'],
There is no active use of defines NO_NATIVES and MIPS_SIMU in the code.

http://codereview.chromium.org/549079/diff/1/67#newcode445
SConstruct:445: 'arch:mips': {
Change this section like this:

      'CPPDEFINES':   ['V8_TARGET_ARCH_MIPS'],
      'simulator:none': {
        'CCFLAGS':      ['-EL', '-mips32r2', '-Wa,-mips32r2', '-fno-inline'],
        'LINKFLAGS':    ['-EL'],
        'LDFLAGS':      ['-EL']
      }

http://codereview.chromium.org/549079/diff/1/67#newcode451
SConstruct:451: 'arch:mips-simu': {
With the change to arch:mips above the pseudo arch mips-simu is no longer
needed.

http://codereview.chromium.org/549079/diff/1/67#newcode796
SConstruct:796: if (self.options['simulator'] == 'mips' and value == 'mips'):
value = 'mips-simu'
This seems wrong. It will overwrite both the value of 'arch' and the value of
'simulator'. This in turn causes the gcc section simulator:mips to be ignored so
the -m32 option are missing when compiling on a 64-bit linux.

With changes above this should not be needed any more.

http://codereview.chromium.org/549079/diff/1/67#newcode859
SConstruct:859: #      options['arch'] = 'mips'
Chis change should not be needed when the pseudo architecture mips-simu gets
removed.

[antonm, 2010-01-21 13:10:45]

Just stylistic comments.

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

http://codereview.chromium.org/549079/diff/1/31#newcode116
src/mips/assembler-mips-inl.h:116: return Assembler::target_address_at(pc_);
nit: wrong indent, -2 spaces.

http://codereview.chromium.org/549079/diff/1/31#newcode140
src/mips/assembler-mips-inl.h:140: return
Handle<Object>(reinterpret_cast<Object**>(Assembler::target_address_at(pc_)));
not: line is too long, ./tools/presubmit.py should have let you known that

http://codereview.chromium.org/549079/diff/1/31#newcode197
src/mips/assembler-mips-inl.h:197: printf("%s - %d - %s : Checking for jal or
jalr. \
I think PrintF should be preferred.  And probably in #ifdef DEBUG

http://codereview.chromium.org/549079/diff/1/31#newcode203
src/mips/assembler-mips-inl.h:203: );
slightly strange indentation, I'd lift ); to the previous line.

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

http://codereview.chromium.org/549079/diff/1/51#newcode312
src/mips/assembler-mips.cc:312: // Checks if the instruction is a branch or a
jump (jr and jalr excluded)
I think this line should be indented as well.

http://codereview.chromium.org/549079/diff/1/51#newcode338
src/mips/assembler-mips.cc:338: int32_t imm18 = (((int32_t)instr &
(int32_t)Imm16Mask) << 16) >>14;
nit: a space between >> and 14

http://codereview.chromium.org/549079/diff/1/51#newcode589
src/mips/assembler-mips.cc:589: if(rt.is_reg()) {
nit: a space between if and (

http://codereview.chromium.org/549079/diff/1/51#newcode654
src/mips/assembler-mips.cc:654: if(rt.is_reg()) {
nit: space between if and (

http://codereview.chromium.org/549079/diff/1/51#newcode675
src/mips/assembler-mips.cc:675: // Signed comparison
nit: -2 spaces

http://codereview.chromium.org/549079/diff/1/51#newcode693
src/mips/assembler-mips.cc:693: // Unsigned comparison.
ditto

http://codereview.chromium.org/549079/diff/1/51#newcode745
src/mips/assembler-mips.cc:745: // Signed comparison
ditto

http://codereview.chromium.org/549079/diff/1/51#newcode767
src/mips/assembler-mips.cc:767: // Unsigned comparison.
ditto

http://codereview.chromium.org/549079/diff/1/51#newcode797
src/mips/assembler-mips.cc:797: if(rt.is_reg()) {
if (

http://codereview.chromium.org/549079/diff/1/51#newcode819
src/mips/assembler-mips.cc:819: // Signed comparison
ditto

http://codereview.chromium.org/549079/diff/1/51#newcode841
src/mips/assembler-mips.cc:841: // Unsigned comparison.
ditto

http://codereview.chromium.org/549079/diff/1/51#newcode939
src/mips/assembler-mips.cc:939: if(!MustUse_at(rt.rmode_)) {
if (.  And below

http://codereview.chromium.org/549079/diff/1/51#newcode940
src/mips/assembler-mips.cc:940: emit(J | rt.imm32_);
+2 spaces

http://codereview.chromium.org/549079/diff/1/51#newcode1320
src/mips/assembler-mips.cc:1320: // Load, store, move
why additional indenting?

http://codereview.chromium.org/549079/diff/1/51#newcode1699
src/mips/assembler-mips.cc:1699: //  return
Memory::Address_at(target_address_address_at(pc));
remove?

http://codereview.chromium.org/549079/diff/1/51#newcode1722
src/mips/assembler-mips.cc:1722: // We should never get here.
UNREACHABLE?

http://codereview.chromium.org/549079/diff/1/51#newcode1724
src/mips/assembler-mips.cc:1724: 
remove empty line?

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

http://codereview.chromium.org/549079/diff/1/58#newcode69
src/mips/assembler-mips.h:69: return 0 <= code_ && code_ < 32; }
nit: I think v8 style would be { ... } on one line or

bool is_valid() const {
  ...
}

http://codereview.chromium.org/549079/diff/1/58#newcode252
src/mips/assembler-mips.h:252: // Class MemOperand represents a memory operand
in load and store instructions
Usually all comments are terminated with a dot (here and everywhere.)

http://codereview.chromium.org/549079/diff/1/58#newcode331
src/mips/assembler-mips.h:331: // This sets the branch destination (which is in
the instruction on x86).
x86?

http://codereview.chromium.org/549079/diff/1/58#newcode368
src/mips/assembler-mips.h:368: //  void beql  (Register rs, Register rt, int16_t
offset);
maybe those commented out lines should be just removed?

http://codereview.chromium.org/549079/diff/1/28
File src/mips/codegen-mips-inl.h (right):

http://codereview.chromium.org/549079/diff/1/28#newcode2
src/mips/codegen-mips-inl.h:2: // Redistribution and use in source and binary
forms, with or without
I don't know what others would say, but maybe you don't need those -inl.h
files---this style gets deprecated.

But consistency may trump (my) comfort in this case.

http://codereview.chromium.org/549079/diff/1/36
File src/mips/constants-mips.cc (right):

http://codereview.chromium.org/549079/diff/1/36#newcode133
src/mips/constants-mips.cc:133: return true;
ditto

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

http://codereview.chromium.org/549079/diff/1/54#newcode401
src/mips/constants-mips.h:401: break;
why break after return?

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

http://codereview.chromium.org/549079/diff/1/52#newcode1
src/mips/test-interface-mips.cc:1: #include <stdint.h>
shouldn't those be moved into test dir?

[Alexandre, 2010-01-22 23:08:42]

Hi,
I have answered and corrected these issues.
They should be fixed for my next upload.

Thanks to the reviewers.

Alexandre

http://codereview.chromium.org/549079/diff/1/67
File SConstruct (right):

http://codereview.chromium.org/549079/diff/1/67#newcode176
SConstruct:176: 'arch:mips': {
Applied this change.
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> If you change this section to
> 
>       'CPPDEFINES':   ['V8_TARGET_ARCH_MIPS'],
>       'simulator:none': {
>         'CCFLAGS':      ['-EL', '-mips32r2', '-Wa,-mips32r2', '-fno-inline'],
>         'LDFLAGS':      ['-EL']
>       }
> 
> The flags for the MIPS gcc will now only become active when not compiling for
> the simulator.

http://codereview.chromium.org/549079/diff/1/67#newcode185
SConstruct:185: 'arch:mips-simu': {
mips-simu arch removed
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> With the change to arch:mips above the pseudo arch mips-simu is no longer
> needed.

http://codereview.chromium.org/549079/diff/1/67#newcode300
SConstruct:300: 'arch:mips-simu': {
mips-simu arch removed
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> With the change to arch:mips above the pseudo arch mips-simu is no longer
> needed.

http://codereview.chromium.org/549079/diff/1/67#newcode301
SConstruct:301: 'CPPDEFINES':   ['V8_TARGET_ARCH_MIPS', 'NO_NATIVES',
'MIPS_SIMU'],
Removed
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> There is no active use of defines NO_NATIVES and MIPS_SIMU in the code.

http://codereview.chromium.org/549079/diff/1/67#newcode445
SConstruct:445: 'arch:mips': {
Applied the changes.
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> Change this section like this:
> 
>       'CPPDEFINES':   ['V8_TARGET_ARCH_MIPS'],
>       'simulator:none': {
>         'CCFLAGS':      ['-EL', '-mips32r2', '-Wa,-mips32r2', '-fno-inline'],
>         'LINKFLAGS':    ['-EL'],
>         'LDFLAGS':      ['-EL']
>       }

http://codereview.chromium.org/549079/diff/1/67#newcode451
SConstruct:451: 'arch:mips-simu': {
mips-simu arch removed
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> With the change to arch:mips above the pseudo arch mips-simu is no longer
> needed.

http://codereview.chromium.org/549079/diff/1/67#newcode796
SConstruct:796: if (self.options['simulator'] == 'mips' and value == 'mips'):
value = 'mips-simu'
Removed.
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> This seems wrong. It will overwrite both the value of 'arch' and the value of
> 'simulator'. This in turn causes the gcc section simulator:mips to be ignored
so
> the -m32 option are missing when compiling on a 64-bit linux.
> 
> With changes above this should not be needed any more.

http://codereview.chromium.org/549079/diff/1/67#newcode859
SConstruct:859: #      options['arch'] = 'mips'
Removed.
On 2010/01/20 15:30:28, Søren Gjesse wrote:
> Chis change should not be needed when the pseudo architecture mips-simu gets
> removed.

http://codereview.chromium.org/549079/diff/1/5
File src/allocation.cc (right):

http://codereview.chromium.org/549079/diff/1/5#newcode109
src/allocation.cc:109: void PreallocatedStorage::Init(size_t size) {
Extra line deleted.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please revert this.

http://codereview.chromium.org/549079/diff/1/22
File src/assembler.cc (right):

http://codereview.chromium.org/549079/diff/1/22#newcode55
src/assembler.cc:55: #elif V8_TARGET_ARCH_MIPS
Removed this part. We now need regexp=interpreted when building for mips.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> I think we can safely ignore the native code RegExp implementation for now.
Use
> regexp=interpreted when building or change the SConstruct file to default
regexp
> to interpreted when building for mips.

http://codereview.chromium.org/549079/diff/1/22#newcode648
src/assembler.cc:648: #elif V8_TARGET_ARCH_MIPS
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> See above.

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

http://codereview.chromium.org/549079/diff/1/17#newcode512
src/assembler.h:512: static inline uint16_t NumBitsSet(uint32_t x) {
Now returning int. The function was renamed to NumberOfBitsSet.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Rename NumBitsSet to NumberOfBitsSet.
> 
> How about just returning int?

http://codereview.chromium.org/549079/diff/1/17#newcode514
src/assembler.h:514: for(int i=0; i<32; i++) {
Style issue fixed. The body function was changed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please add spaces like this:
> 
> for (int i = 0; i < 32; i++) {
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo....
> 
> I don't know whether the performance of this function is important, but take a
> look at
> http://www-graphics.stanford.edu/%7Eseander/bithacks.html#CountBitsSetNaive
for
> alternatives.

http://codereview.chromium.org/549079/diff/1/17#newcode515
src/assembler.h:515: if(x & 1<<i) num_bits_set++;
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Add spaces here like this
> 
> if (x & (1 << i)) num_bits_set++;

http://codereview.chromium.org/549079/diff/1/64
File src/checks.h (left):

http://codereview.chromium.org/549079/diff/1/64#oldcode54
src/checks.h:54: 
Reverted.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please revert this.

http://codereview.chromium.org/549079/diff/1/11
File src/codegen.h (right):

http://codereview.chromium.org/549079/diff/1/11#newcode153
src/codegen.h:153: #ifndef V8_TARGET_ARCH_MIPS
This is necessary for MIPS. We don't have a cmp instruction and conditional
execution, but rather conditional branch instructions. I see two ways to get rid
of this but neither is good:
1) Make all architecture use the mips template. Except for mips they would not
use the extra arguments. Not so good...
2) On MIPS always setup the values to be compared in the same registers and pass
some variable saying which condition to test. This would be really inefficient.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Having a platform specific #ifdef in a platform independent file should be
> avoided.

http://codereview.chromium.org/549079/diff/1/11#newcode156
src/codegen.h:156: inline void Branch(Condition cc, Register src1 = zero_reg,
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Look at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> forformatting of parameter list.

http://codereview.chromium.org/549079/diff/1/12
File src/compiler.cc (right):

http://codereview.chromium.org/549079/diff/1/12#newcode123
src/compiler.cc:123: #ifndef V8_TARGET_ARCH_MIPS
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please get rid of this #ifdef. You can create a FastCodeGenerator for MIPS
which
> does bailout in all cases.

http://codereview.chromium.org/549079/diff/1/26
File src/disasm.h (right):

http://codereview.chromium.org/549079/diff/1/26#newcode42
src/disasm.h:42: #ifdef V8_TARGET_ARCH_MIPS
Updated. We now use this function instead of NameOfCOP1Register.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> This #ifdef should not be needed. Platform specific stuff has already spilled
> into this file (NameOfXMMRegister). That is not good practice, but not your
> fault.

http://codereview.chromium.org/549079/diff/1/8
File src/flag-definitions.h (right):

http://codereview.chromium.org/549079/diff/1/8#newcode208
src/flag-definitions.h:208: DEFINE_bool(trace_sim, true, "trace simulator
execution")
Set back to the default value.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please keep the default to false.

http://codereview.chromium.org/549079/diff/1/66
File src/globals.h (right):

http://codereview.chromium.org/549079/diff/1/66#newcode60
src/globals.h:60: #define V8_TARGET_CAN_READ_UNALIGNED 0
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> defining V8_TARGET_CAN_READ_UNALIGNED to 0 should not be needed.

http://codereview.chromium.org/549079/diff/1/66#newcode180
src/globals.h:180: // On mips 0xbaddead is the encoding of jump to 0xeb77ab4.
I wrote this in the early development stage. It can safely be removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> I don't think this is important. The handle zap value is placed where object
> pointers used to be, and will most likely be used as an object value and not
> executed code. Getting a segfault should be fine.

http://codereview.chromium.org/549079/diff/1/4
File src/heap.cc (right):

http://codereview.chromium.org/549079/diff/1/4#newcode47
src/heap.cc:47: #elif V8_TARGET_ARCH_MIPS && V8_NATIVE_REGEXP
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> I think you can safely ignore native RegExp for MIPS for now.

http://codereview.chromium.org/549079/diff/1/4#newcode1338
src/heap.cc:1338: 
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Accidental edit?

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

http://codereview.chromium.org/549079/diff/1/63#newcode53
src/jsregexp.cc:53: #elif V8_TARGET_ARCH_MIPS
Removed
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> I think you can safely ignore native RegExp for MIPS for now.

http://codereview.chromium.org/549079/diff/1/63#newcode4481
src/jsregexp.cc:4481: #elif V8_TARGET_ARCH_MIPS
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> I think you can safely ignore native RegExp for MIPS for now.

http://codereview.chromium.org/549079/diff/1/23
File src/jump-target.h (right):

http://codereview.chromium.org/549079/diff/1/23#newcode116
src/jump-target.h:116: #ifndef V8_TARGET_ARCH_MIPS
Same comment (see codegen.h). We need this on MIPS.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Like in codegen.h having a platform specific #ifdef in a platform independent
> file should be avoided.

http://codereview.chromium.org/549079/diff/1/23#newcode120
src/jump-target.h:120: virtual void Branch(Condition cc, Hint hint = no_hint,
Register src1 = zero_reg, const Operand& src2 = Operand(zero_reg));
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Long line, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Line_L....

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

http://codereview.chromium.org/549079/diff/1/31#newcode116
src/mips/assembler-mips-inl.h:116: return Assembler::target_address_at(pc_);
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> nit: wrong indent, -2 spaces.

http://codereview.chromium.org/549079/diff/1/31#newcode140
src/mips/assembler-mips-inl.h:140: return
Handle<Object>(reinterpret_cast<Object**>(Assembler::target_address_at(pc_)));
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> not: line is too long, ./tools/presubmit.py should have let you known that

http://codereview.chromium.org/549079/diff/1/31#newcode197
src/mips/assembler-mips-inl.h:197: printf("%s - %d - %s : Checking for jal or
jalr. \
Replaced printf with PrintF.
On 2010/01/21 13:10:45, antonm wrote:
> I think PrintF should be preferred.  And probably in #ifdef DEBUG

http://codereview.chromium.org/549079/diff/1/31#newcode203
src/mips/assembler-mips-inl.h:203: );
Style issue fixed.
Also removed magic numbers.
On 2010/01/21 13:10:45, antonm wrote:
> slightly strange indentation, I'd lift ); to the previous line.

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

http://codereview.chromium.org/549079/diff/1/51#newcode1
src/mips/assembler-mips.cc:1: // Copyright 2006-2008 the V8 project authors. All
rights reserved.
Replaced the copyright.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please use the copyright header from src/mips/assembler-mips-inl.h mentioning
> Sun Microsystems Inc. here as well.

http://codereview.chromium.org/549079/diff/1/51#newcode312
src/mips/assembler-mips.cc:312: // Checks if the instruction is a branch or a
jump (jr and jalr excluded)
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> I think this line should be indented as well.

http://codereview.chromium.org/549079/diff/1/51#newcode338
src/mips/assembler-mips.cc:338: int32_t imm18 = (((int32_t)instr &
(int32_t)Imm16Mask) << 16) >>14;
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> nit: a space between >> and 14

http://codereview.chromium.org/549079/diff/1/51#newcode589
src/mips/assembler-mips.cc:589: if(rt.is_reg()) {
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> nit: a space between if and (

http://codereview.chromium.org/549079/diff/1/51#newcode654
src/mips/assembler-mips.cc:654: if(rt.is_reg()) {
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> nit: space between if and (

http://codereview.chromium.org/549079/diff/1/51#newcode675
src/mips/assembler-mips.cc:675: // Signed comparison
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> nit: -2 spaces

http://codereview.chromium.org/549079/diff/1/51#newcode693
src/mips/assembler-mips.cc:693: // Unsigned comparison.
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> ditto

http://codereview.chromium.org/549079/diff/1/51#newcode745
src/mips/assembler-mips.cc:745: // Signed comparison
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> ditto

http://codereview.chromium.org/549079/diff/1/51#newcode767
src/mips/assembler-mips.cc:767: // Unsigned comparison.
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> ditto

http://codereview.chromium.org/549079/diff/1/51#newcode797
src/mips/assembler-mips.cc:797: if(rt.is_reg()) {
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> if (

http://codereview.chromium.org/549079/diff/1/51#newcode819
src/mips/assembler-mips.cc:819: // Signed comparison
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> ditto

http://codereview.chromium.org/549079/diff/1/51#newcode939
src/mips/assembler-mips.cc:939: if(!MustUse_at(rt.rmode_)) {
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> if (.  And below

http://codereview.chromium.org/549079/diff/1/51#newcode940
src/mips/assembler-mips.cc:940: emit(J | rt.imm32_);
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> +2 spaces

http://codereview.chromium.org/549079/diff/1/51#newcode1320
src/mips/assembler-mips.cc:1320: // Load, store, move
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> why additional indenting?

http://codereview.chromium.org/549079/diff/1/51#newcode1699
src/mips/assembler-mips.cc:1699: //  return
Memory::Address_at(target_address_address_at(pc));
Removed.
On 2010/01/21 13:10:45, antonm wrote:
> remove?

http://codereview.chromium.org/549079/diff/1/51#newcode1722
src/mips/assembler-mips.cc:1722: // We should never get here.
Added UNREACHABLE() macro.
On 2010/01/21 13:10:45, antonm wrote:
> UNREACHABLE?

http://codereview.chromium.org/549079/diff/1/51#newcode1724
src/mips/assembler-mips.cc:1724: 
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> remove empty line?

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

http://codereview.chromium.org/549079/diff/1/58#newcode1
src/mips/assembler-mips.h:1: // Copyright 2006-2008 the V8 project authors. All
rights reserved.
Changed the copyright.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please use the copyright header from src/mips/assembler-mips-inl.h mentioning
> Sun Microsystems Inc. here as well.

http://codereview.chromium.org/549079/diff/1/58#newcode69
src/mips/assembler-mips.h:69: return 0 <= code_ && code_ < 32; }
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> nit: I think v8 style would be { ... } on one line or
> 
> bool is_valid() const {
>   ...
> }

http://codereview.chromium.org/549079/diff/1/58#newcode252
src/mips/assembler-mips.h:252: // Class MemOperand represents a memory operand
in load and store instructions
Style issue fixed.
On 2010/01/21 13:10:45, antonm wrote:
> Usually all comments are terminated with a dot (here and everywhere.)

http://codereview.chromium.org/549079/diff/1/58#newcode331
src/mips/assembler-mips.h:331: // This sets the branch destination (which is in
the instruction on x86).
Fixed.
On 2010/01/21 13:10:45, antonm wrote:
> x86?

http://codereview.chromium.org/549079/diff/1/58#newcode368
src/mips/assembler-mips.h:368: //  void beql  (Register rs, Register rt, int16_t
offset);
I removed them. These likely variants of instructions should not be used.
On 2010/01/21 13:10:45, antonm wrote:
> maybe those commented out lines should be just removed?

http://codereview.chromium.org/549079/diff/1/30
File src/mips/cfg-mips.cc (right):

http://codereview.chromium.org/549079/diff/1/30#newcode1
src/mips/cfg-mips.cc:1: //
Removed
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> You can safely remove this file as using a control flow graph was an
experiment
> which has been abandonned.

http://codereview.chromium.org/549079/diff/1/28
File src/mips/codegen-mips-inl.h (right):

http://codereview.chromium.org/549079/diff/1/28#newcode2
src/mips/codegen-mips-inl.h:2: // Redistribution and use in source and binary
forms, with or without
Well I personnally don't care much about this. However tho files (.h and .cc)
would seem enough to me.
On 2010/01/21 13:10:45, antonm wrote:
> I don't know what others would say, but maybe you don't need those -inl.h
> files---this style gets deprecated.
> 
> But consistency may trump (my) comfort in this case.

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

http://codereview.chromium.org/549079/diff/1/34#newcode1
src/mips/codegen-mips.cc:1: // Copyright 2006-2008 the V8 project authors. All
rights reserved.
Removed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please remove all the code in comments.

http://codereview.chromium.org/549079/diff/1/49
File src/mips/codegen-mips.h (right):

http://codereview.chromium.org/549079/diff/1/49#newcode466
src/mips/codegen-mips.h:466: //    // Encode the parameters in a unique 16 bit
value.
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Please remove code in comments.

http://codereview.chromium.org/549079/diff/1/49#newcode477
src/mips/codegen-mips.h:477: //    if (constant_rhs ==
CodeGenerator::kUnknownIntValue) return false;
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/549079/diff/1/49#newcode490
src/mips/codegen-mips.h:490: //    if (!specialized_on_rhs_) return 0;
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/549079/diff/1/49#newcode522
src/mips/codegen-mips.h:522: //    if (!specialized_on_rhs_) {
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Ditto.

http://codereview.chromium.org/549079/diff/1/36
File src/mips/constants-mips.cc (right):

http://codereview.chromium.org/549079/diff/1/36#newcode133
src/mips/constants-mips.cc:133: return true;
Fixed.
On 2010/01/21 13:10:45, antonm wrote:
> ditto

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

http://codereview.chromium.org/549079/diff/1/54#newcode13
src/mips/constants-mips.h:13: // (
http://web.asic.sdesigns.com/tools/libs/mips/doc/74K/doc/MIPS32-ISA.pdf )
Added a functional link and a comment. I doubt it will generate too much traffic
but maybe the link should be removed?
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> This link seems not to work. How about using a http://www.mips.com URL? Even
if it
> requires login.

http://codereview.chromium.org/549079/diff/1/54#newcode112
src/mips/constants-mips.h:112: J         =   ( (0<<3) + 2 ) << opcode_o,
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Formatting like ((0 << 3) + 2) << opcode_o matches the rest of the V8 code
> better.

http://codereview.chromium.org/549079/diff/1/54#newcode128
src/mips/constants-mips.h:128: COP1      =   ( (2<<3) + 1 ) << opcode_o,   //
Coprocessor 1 class (floating point)
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Long line.

http://codereview.chromium.org/549079/diff/1/54#newcode350
src/mips/constants-mips.h:350: return Bits(rs_o+rs_l-1, rs_o);
Style issue fixed.
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Format expression with spaces rs_o + rs_l - 1.

http://codereview.chromium.org/549079/diff/1/54#newcode401
src/mips/constants-mips.h:401: break;
Fixed.
On 2010/01/21 13:10:45, antonm wrote:
> why break after return?

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

http://codereview.chromium.org/549079/diff/1/52#newcode1
src/mips/test-interface-mips.cc:1: #include <stdint.h>
Yes they should. I'll remove it.
On 2010/01/21 13:10:45, antonm wrote:
> shouldn't those be moved into test dir?

http://codereview.chromium.org/549079/diff/1/19
File src/platform-linux.cc (right):

http://codereview.chromium.org/549079/diff/1/19#newcode98
src/platform-linux.cc:98: #ifdef V8_TARGET_ARCH_MIPS
Fixed
On 2010/01/19 22:59:12, Søren Gjesse wrote:
> Combine with #else above to #elif.

[Søren Thygesen Gjesse, 2010-02-15 10:19:58]

Closing this issue as it was moved to http://codereview.chromium.org/561072.