On Fri, Feb 26, 2010 at 1:07 PM, <[email protected]> wrote: > > 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 !=). >
fixed > > 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. > fixed > > 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. > > fixed > 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. > > fixed > 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). > > fixed > 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. > Any suggestions? Note that the bit patterns do not match the ARM bit patters for data processing operations, that constants currently can't be shared with the simulator, and for the 16 bit thumb instructions the composition is even more irregular. > http://codereview.chromium.org/651029/diff/3020/4058#newcode651 > src/arm/assembler-thumb2.cc:651: ASSERT(false); > UNREACHABLE() or UNIMPLEMENTED(). Also below. > fixed 2x > > http://codereview.chromium.org/651029/diff/3020/4058#newcode717 > src/arm/assembler-thumb2.cc:717: > Spurious edit. > empty line removed > > 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. > > changed indent to 4 > 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? Currently by always switching back to ARM mode immediately. > 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. > changed > > 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. > changed > > http://codereview.chromium.org/651029/diff/3020/4052#newcode1176 > src/arm/assembler-thumb2.h:1176: Register rm, ShiftOp shiftOp, int > shiftBy); > Funky indentation. > > removed one space in both cases > > 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. > Not sure what exactly you are referring to here with initialization costs? > I suggest that you leave instr0_ and instr1_ and make all the rest inlined accessor functions. I don't think inlining the decoding logic will be more efficient, and I think it will be hard to write it in a readable form. See http://www.gbadev.org/docs.php?showinfo=19 for an overview of the composition of the 16 bit opcodes. One option that would use less fields would be to extract the operation and a named "instruction pattern" in the first step, and extract rm, rn, imm etc. on demand. But that would mean a separate switch/case for each access to rm, rn etc, so I think it would have a negative impact on performance. http://codereview.chromium.org/651029 -- Stefan Haustein Google UK Limited Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ; Registered in England Number: 3977902 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
