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. http://codereview.chromium.org/651029 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
