Forking disassembler and simulator for Thumb2 support;

changed as discussed on v8-dev. Needed to re-upload due to issues with my SVN client.

[haustein, 2010-02-23 14:03:56]

changed as discussed on v8-dev. Needed to re-upload due to issues with my SVN
client (previously http://codereview.chromium.org/601028 )

[Erik Corry, 2010-02-23 15:10:31]

http://codereview.chromium.org/651029/diff/1006/48
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/651029/diff/1006/48#newcode563
src/arm/simulator-arm.cc:563: return v + ((v & 1) ? + Instr::kPCReadOffsetThumb
: Instr::kPCReadOffset);
There's a stray '+' here and the (v & 1) thing needs to be in its own method
called IsInThumbMode.  When you do that method remember not to use implicit bool
conversion.

http://codereview.chromium.org/651029/diff/1006/48#newcode2159
src/arm/simulator-arm.cc:2159: result >>= (shift_amount - 1);
Can shift_amount be zero?  If so how does this work.  If not, then please assert
that it is not zero.

http://codereview.chromium.org/651029/diff/1006/48#newcode2204
src/arm/simulator-arm.cc:2204: //  enter_debug_ = true;
Please no commented out code.

http://codereview.chromium.org/651029/diff/1006/48#newcode2320
src/arm/simulator-arm.cc:2320: } else if (program_counter & 1) {
Why only if program_counter & 1?
(And implicit conversion)

[Erik Corry, 2010-02-24 12:28:58]

http://codereview.chromium.org/651029/diff/1006/55
File src/arm/instr-thumb2.cc (right):

http://codereview.chromium.org/651029/diff/1006/55#newcode40
src/arm/instr-thumb2.cc:40: if (Bits0(15, 13) == b111 && Bits0(12, 11) != b00) {
We don't feel these bxxx constants add enough in terms of clarity to compensate
for their strangeness.  Please use hex throughout.  If possible of course the
constants should have real names.  Any opcode in table A6-1 and the tables on
the following pages should have a name.  In this case it would be clearer to
write:

if (Bits(15, 11) > kMax16BitThumbOpcode)

where kMax16BitThumbOpcode is 0x1c, from table A6-1.

[Mads Ager (chromium), 2010-02-26 08:50:01]

A couple of overall comments.  I'll let Erik review the actual functionality.

http://codereview.chromium.org/651029/diff/3020/4046
File src/SConscript (right):

http://codereview.chromium.org/651029/diff/3020/4046#newcode122
src/SConscript:122: arm/instr-thumb2.cc
Can we get rid of this new file please?  We should follow the code organization
from the other assemblers so that things are not different for just one of our
backends.

http://codereview.chromium.org/651029/diff/3020/4056
File src/arm/assembler-thumb2-inl.h (right):

http://codereview.chromium.org/651029/diff/3020/4056#newcode150
src/arm/assembler-thumb2-inl.h:150: return (Assembler::instr_arm_at(pc_) ==
kMovLrPc) &&
Funky indentation here.  This is not your code, but I think we should rewrite
this overly complex return statement.  Let's do something like (and change it in
the other assembler as well):

if (Assembler::instr_arm_at(pc_) == kMovLrPc) {
  Instr instr = Assembler::instr_arm_at(pc_ + Assembler::kInstrArmSize);
  return (instr & kLdrPcPattern) == kLdrPcPattern;
}
return false;

http://codereview.chromium.org/651029/diff/3020/4052
File src/arm/assembler-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4052#newcode104
src/arm/assembler-thumb2.h:104: enum Bits1 {
I would like to get rid of these Bits enums.

http://codereview.chromium.org/651029/diff/3020/4053
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/651029/diff/3020/4053#newcode132
src/arm/constants-arm.h:132: enum Bits1 {
Let's get rid of these.

http://codereview.chromium.org/651029/diff/3020/4057
File src/arm/instr-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4057#newcode45
src/arm/instr-thumb2.h:45: enum Operation {
Why do you need this enum?  There is nothing like this in the normal ARM
assembler and we should not diverge from that unless we really have to.

http://codereview.chromium.org/651029/diff/3020/4057#newcode135
src/arm/instr-thumb2.h:135: class InstrThumb2 {
Again, it seems to me that we should follow the style of the other assemblers. 
Having something different in the thumb2 assembler will just be confusing when
developing for all platforms.

I think we should get rid of these new files and follow the style of the other
assemblers.

[Erik Corry, 2010-02-26 13:07:22]

http://codereview.chromium.org/651029/diff/3020/4056
File src/arm/assembler-thumb2-inl.h (right):

http://codereview.chromium.org/651029/diff/3020/4056#newcode212
src/arm/assembler-thumb2-inl.h:212: if (pc_offset() & 2 != 0) {
This doesn't do what you think it does (precedence of & and !=).

http://codereview.chromium.org/651029/diff/3020/4056#newcode214
src/arm/assembler-thumb2-inl.h:214: // Pad with a T1 NOP
Comments should end with full stops throughout.  Please use 'thumb' instead of
the abbreviation 'T1' for clarity.

http://codereview.chromium.org/651029/diff/3020/4056#newcode227
src/arm/assembler-thumb2-inl.h:227: ASSERT((pc_offset() & 1) == 0);
Please add a comment explaining where the '3' comes from.

http://codereview.chromium.org/651029/diff/3020/4058
File src/arm/assembler-thumb2.cc (right):

http://codereview.chromium.org/651029/diff/3020/4058#newcode612
src/arm/assembler-thumb2.cc:612: 
Two blank lines between functions.

http://codereview.chromium.org/651029/diff/3020/4058#newcode624
src/arm/assembler-thumb2.cc:624: // the label points to an arm instruction
Punctuation (ARM is all caps).

http://codereview.chromium.org/651029/diff/3020/4058#newcode641
src/arm/assembler-thumb2.cc:641: i0 = b11110 * B11 | b1000 * B5;
Instead of bxxxx we should have meaningful names here.

http://codereview.chromium.org/651029/diff/3020/4058#newcode651
src/arm/assembler-thumb2.cc:651: ASSERT(false);
UNREACHABLE() or UNIMPLEMENTED().  Also below.

http://codereview.chromium.org/651029/diff/3020/4058#newcode717
src/arm/assembler-thumb2.cc:717: 
Spurious edit.

http://codereview.chromium.org/651029/diff/3020/4058#newcode1159
src/arm/assembler-thumb2.cc:1159: instr_arm_at(pc_ - 2 * kInstrArmSize) ==
The line above looks indented too much.

http://codereview.chromium.org/651029/diff/3020/4058#newcode1885
src/arm/assembler-thumb2.cc:1885: emit_int32(0x03000000 | num_prinfo_);
Do you ensure that you are 4 byte aligned before doing this?

http://codereview.chromium.org/651029/diff/3020/4052
File src/arm/assembler-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4052#newcode642
src/arm/assembler-thumb2.h:642: // hack to keep the code patcher happy for now
Comments should start with a capital letter.

http://codereview.chromium.org/651029/diff/3020/4052#newcode1141
src/arm/assembler-thumb2.h:1141: static const int kMaxNumPRInfo =
kMaxDistBetweenPools/kInstrArmSize;
Not your fault, but this / should be surrounded by spaces.

http://codereview.chromium.org/651029/diff/3020/4052#newcode1176
src/arm/assembler-thumb2.h:1176: Register rm, ShiftOp shiftOp, int shiftBy);
Funky indentation.

http://codereview.chromium.org/651029/diff/3020/4057
File src/arm/instr-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4057#newcode229
src/arm/instr-thumb2.h:229: int imm_;
The instruction needs to be a very lightweight object.  This thing has 15
fields, most of which will be unused in most instructions.  Just initializing
this object is likely to cost you.

I suggest that you leave instr0_ and instr1_ and make all the rest inlined
accessor functions.

[Erik Corry, 2010-03-02 14:03:55]

Response to question posted through email, not code review tool:

http://codereview.chromium.org/651029/diff/3020/4058
File src/arm/assembler-thumb2.cc (right):

http://codereview.chromium.org/651029/diff/3020/4058#newcode641
src/arm/assembler-thumb2.cc:641: i0 = b11110 * B11 | b1000 * B5;
On 2010/02/26 13:07:23, Erik Corry wrote:
> Instead of bxxxx we should have meaningful names here.

You asked for suggestions, so here they are:

The table on page A6-6 turns into the following:

enum ThumbEncoding {
  kThumbRegularEncodingStart = 0x0,
  kThumbRegularEncodingEnd = 0xf,
  kThumbDataProcessingEncoding = 0x10,
  kThumbSpecialEncoding = 0x11,
  kThumbLoadLiteralEncodingStart = 0x12,
  kThumbLoadLiteralEncodingEnd = 0x13,
  kThumbLoadStoreSingleEncodingStart = 0x14,
  kThumbLoadStoreSingleEncodingEnd = 0x27,
  kThumbGeneratePCRelativeEncodingStart = 0x28,
  kThumbGeneratePCRelativeEncodingEnd = 0x29,
  kThumbGenerateSPRelativeEncodingStart = 0x2a,
  kThumbGenerateSPRelativeEncodingEnd = 0x2b,
  kThumbMiscEncodingStart = 0x2c,
  kThumbMiscEncodingEnd = 0x2f,
  kThumbStoreMultipleEncodingStart = 0x30,
  kThumbStoreMultipleEncodingEnd = 0x31,
  kThumbLoadMultipleEncodingStart = 0x32,
  kThumbLoadMultipleEncodingEnd = 0x33,
  kThumbConditionalBranchEncodingStart = 0x34,
  kThumbConditionalBranchEncodingEnd = 0x37,
  kThumbUnconditionalBranchEncodingStart = 0x38,
  kThumbUnconditionalBranchEncodingEnd = 0x39,
  kThumb32BitEncodingStart = 0x3a,
  kThumb32BitEncodingEnd = 0x3f
};

The table on page A6-14 yields:

enum Thumb32Encoding {
  kThumb32LoadStoreMultipleEncoding = 0x80,
  kThumb32LoadStoreDualEncoding = 0x84,
  kThumb32ShiftedRegisterEncoding = 0xa0,
  ...
  kThumb32ModifiedImmediateEncoding = 0x100,
  kThumb32PlainImmediateencoding = 0x140,
  ...
};

enum ThumbModifiedImmediateOpcode {
  kModifiedImmediateAnd = 0,
  kModifiedImmediateTest = 0,
  kModifiedImmediateBic = 1,
  kModifiedImmediateOr = 2,
  ...
  kModifiedImmediateAdd = 8,
  ...
};

So now the above becomes:

i0 = kThumb32BitEncodingStart * B10 |
     kThumb32ModifiedImmediateEncoding * B4 |
     kModifiedImmediateAdd * B5;

You might consider having the enum values be pre-shifted by the right amount so
you don't have to multiply by B4, B5, etc.
i0 = 
};

[Erik Corry, 2010-03-02 14:15:38]

The stuff from the new .h file can be moved into constants-arm.h.  The stuff
from the new .cc file can go either in the simulator or disassembler .cc file.

http://codereview.chromium.org/651029/diff/3020/4057
File src/arm/instr-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4057#newcode229
src/arm/instr-thumb2.h:229: int imm_;
On 2010/02/26 13:07:23, Erik Corry wrote:
> The instruction needs to be a very lightweight object.  This thing has 15
> fields, most of which will be unused in most instructions.  Just initializing
> this object is likely to cost you.
> 
> I suggest that you leave instr0_ and instr1_ and make all the rest inlined
> accessor functions.

Having thought about this a bit more I can see that the irregularity of the T2
instruction set makes this a reasonable option.

In the slightly longer run we would like to see the T2 and ARM simulators use
the same overall structure.  The easiest way to achieve this would be to move
the ARM instruction decoder to use the same infrastructure.  I don't feel that
has to be a part of this first change list though.

[haustein, 2010-03-02 16:49:46]

Sorry, didn't immediate realize the review tool is not updated from email; will
stick to the tool now.

On 2010/03/02 14:15:38, Erik Corry wrote:
> The stuff from the new .h file can be moved into constants-arm.h.  The stuff
> from the new .cc file can go either in the simulator or disassembler .cc file.
> 

Done. 
Also removed the bxxxx constants; replaced them with hex/Bxx for now:
consolidation of constants.arm.h, assembler-arm.h and assembler-thumb2.h should
probably be a separate CL. 

> http://codereview.chromium.org/651029/diff/3020/4057
> File src/arm/instr-thumb2.h (right):
> 
> http://codereview.chromium.org/651029/diff/3020/4057#newcode229
> src/arm/instr-thumb2.h:229: int imm_;
> On 2010/02/26 13:07:23, Erik Corry wrote:
> > The instruction needs to be a very lightweight object.  This thing has 15
> > fields, most of which will be unused in most instructions.  Just
initializing
> > this object is likely to cost you.
> > 
> > I suggest that you leave instr0_ and instr1_ and make all the rest inlined
> > accessor functions.
> 
> Having thought about this a bit more I can see that the irregularity of the T2
> instruction set makes this a reasonable option.
> 
> In the slightly longer run we would like to see the T2 and ARM simulators use
> the same overall structure.  The easiest way to achieve this would be to move
> the ARM instruction decoder to use the same infrastructure.  I don't feel that
> has to be a part of this first change list though.