Thanks for the patch. Some comments are below. Can we find a name for the feature that isn't ARMv7 since ARMV6T2 has the instruction too?
http://codereview.chromium.org/569015/diff/3008/20 File src/arm/assembler-arm.cc (right): http://codereview.chromium.org/569015/diff/3008/20#newcode866 src/arm/assembler-arm.cc:866: emit(cond | 0xF*B23 | 0x3*B21 | (0x1F & src3.imm32_)*B16 | Is there a reason not to write 0x3F*B21 here? http://codereview.chromium.org/569015/diff/3008/20#newcode867 src/arm/assembler-arm.cc:867: dst.code()*B12 | (0x1F & src2.imm32_)*B7 | 0x5*B4 | src1.code()); Since src2 and src3 can only be immediates you should assert that this is the case and assert that they are in range instead of masking with 0x1f. Also check that the reloc mode is none. http://codereview.chromium.org/569015/diff/3008/19 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/569015/diff/3008/19#newcode6035 src/arm/codegen-arm.cc:6035: if (CpuFeatures::IsSupported(ARMv7)) { This seems wrong since the ARMv6T2 architecture also has the ubfx instruction. http://codereview.chromium.org/569015/diff/3008/22 File src/arm/disasm-arm.cc (right): http://codereview.chromium.org/569015/diff/3008/22#newcode808 src/arm/disasm-arm.cc:808: if ((instr->HasW()) && (instr->HasW()) && (instr->Bits(6, 4) == 0x5)) { We only need to chek the HasW bit once. http://codereview.chromium.org/569015/diff/3008/22#newcode809 src/arm/disasm-arm.cc:809: unsigned int widthminus1 = (unsigned int)(instr->Bits(20, 16)); No C style casts. http://codereview.chromium.org/569015/diff/3008/22#newcode812 src/arm/disasm-arm.cc:812: if (msbit <=31) { Space after <= http://codereview.chromium.org/569015/diff/3008/21 File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/569015/diff/3008/21#newcode1772 src/arm/simulator-arm.cc:1772: unsigned int widthminus1 = (unsigned int)(instr->Bits(20, 16)); Please use int32_t and uint32_t instead of int. http://codereview.chromium.org/569015/diff/3008/21#newcode1775 src/arm/simulator-arm.cc:1775: if (msbit <=31) { space after <= http://codereview.chromium.org/569015/diff/3008/23 File src/globals.h (right): http://codereview.chromium.org/569015/diff/3008/23#newcode610 src/globals.h:610: ARMv7 = 2, // ARMv7 The comment should just say ARM, not ARMv7 http://codereview.chromium.org/569015 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
