ARMv7 ubfx support from Kun Zhang (zhangk@codeaurora.org), Code Aurora

[Mads Ager (chromium), 2010-02-03 07:45:57]

Erik would be the right person to review this.

[Søren Thygesen Gjesse, 2010-02-03 07:56:09]

http://codereview.chromium.org/569015/diff/3008/20
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/569015/diff/3008/20#newcode56
src/arm/assembler-arm.cc:56: supported_ |= 1u << VFP3;
Please add a comment on why ARMv7 is enabled when running the simulator. Also
please add an enable_armv7 flag (flag-definitions.h). Also I think we should
take the flags FLAG_enable_vfp3 and FLAG_enable_armv7 into account when running
the simulator.

http://codereview.chromium.org/569015/diff/3008/19
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/569015/diff/3008/19#newcode6018
src/arm/codegen-arm.cc:6018: // Remove tags from right operand.
Consider refactoring to avoid duplicated code.

http://codereview.chromium.org/569015/diff/3008/19#newcode6020
src/arm/codegen-arm.cc:6020: // UBFX r2, r0, #kSmiTagSize, #(5-1)
This comment is not needed (two more below).

http://codereview.chromium.org/569015/diff/3008/19#newcode6024
src/arm/codegen-arm.cc:6024: // Use only the 5 least significant bits of the
shift count.
Please hoist this comment to above "if (CpuFeatures::IsSupported(ARMv7))..."
(two more below).

http://codereview.chromium.org/569015/diff/3008/21
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/569015/diff/3008/21#newcode1771
src/arm/simulator-arm.cc:1771: if ((instr->HasW()) && (instr->HasW()) &&
(instr->Bits(6, 4) == 0x5)) {
Please only use C++ style casts.

[Erik Corry, 2010-02-03 09:17:56]

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

[Erik Corry, 2010-02-03 10:17:04]

Another thing.  You should update constants-arm.h so it defines a macro
CAN_USE_ARMV6T2_INSTRUCTIONS which is analogous to the
CAN_USE_ARMVx_INSTRUCTIONS that is already there.  Then you should modify
OS::CpuFeaturesImpliedByPlatform in platform-linux.cc so it makes use of the
flag to set the bit that indicates we can use ubfx instructions.  That way if
the C++ compiler is given the armv7 flag (not unlikely on an embedded platform)
we can make use of the instructions without having to check at runtime (this is
important for generated code that is part of the VM in the snapshot).

[zhangk, 2010-02-04 00:29:08]

Thank you for all of the good comments and suggestions. constants-arm.h and
platform-linux.cc are updated with CAN_USE_ARMV7_INSTRUCTIONS since our testing
infrastruction limits the ubfx change to ARMv7.

[zhangk, 2010-02-04 00:31:16]

Thank you for the good points and comments. I revised the code according to the
reviews except one that is due to our testing limitation. If there are other
questions or conserns, please let me know.

http://codereview.chromium.org/569015/diff/3008/20
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/569015/diff/3008/20#newcode56
src/arm/assembler-arm.cc:56: supported_ |= 1u << VFP3;
On 2010/02/03 07:56:09, Søren Gjesse wrote:

Done.

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 |
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

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());
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/19
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/569015/diff/3008/19#newcode6018
src/arm/codegen-arm.cc:6018: // Remove tags from right operand.
On 2010/02/03 07:56:09, Søren Gjesse wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/19#newcode6024
src/arm/codegen-arm.cc:6024: // Use only the 5 least significant bits of the
shift count.
On 2010/02/03 07:56:09, Søren Gjesse wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/19#newcode6035
src/arm/codegen-arm.cc:6035: if (CpuFeatures::IsSupported(ARMv7)) {
We limit ubfx to ARMv7 since we don't have ARMv6T2 hardware to do the testing.
If we have that hardware in the future, we'll add the support for ARMv6T2.

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)) {
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/22#newcode809
src/arm/disasm-arm.cc:809: unsigned int widthminus1 = (unsigned
int)(instr->Bits(20, 16));
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/22#newcode812
src/arm/disasm-arm.cc:812: if (msbit <=31) {
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/21
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/569015/diff/3008/21#newcode1771
src/arm/simulator-arm.cc:1771: if ((instr->HasW()) && (instr->HasW()) &&
(instr->Bits(6, 4) == 0x5)) {
On 2010/02/03 07:56:09, Søren Gjesse wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/21#newcode1772
src/arm/simulator-arm.cc:1772: unsigned int widthminus1 = (unsigned
int)(instr->Bits(20, 16));
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

http://codereview.chromium.org/569015/diff/3008/21#newcode1775
src/arm/simulator-arm.cc:1775: if (msbit <=31) {
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

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
On 2010/02/03 09:17:56, Erik Corry wrote:

Done.

[Søren Thygesen Gjesse, 2010-02-04 07:38:32]

http://codereview.chromium.org/569015/diff/5006/4006
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/569015/diff/5006/4006#newcode5827
src/arm/codegen-arm.cc:5827: void
GenericBinaryOpStub::GetShiftCountLeastBits(MacroAssembler* masm,
Please add register parameters source and destination instead of hardcoding r0
and r2.

I think the function should be called something like GetLeastBitsFromSmi, as
what it really does is getting the num_least_bits from a smi. If something like
this is needed in other places you could consider moving it to the macro
assembler instead.

http://codereview.chromium.org/569015/diff/5006/4006#newcode5831
src/arm/codegen-arm.cc:5831: // UBFX r2, r0, #kSmiTagSize, #(5-1)
Comment assumes num_least_bits == 5.

http://codereview.chromium.org/569015/diff/5006/4006#newcode5833
src/arm/codegen-arm.cc:5833: } else {
This branch does not take num_least_bits into account. Something like 0x1f -> (1
<< num_least_bits) - 1 below.

http://codereview.chromium.org/569015/diff/5006/4006#newcode5834
src/arm/codegen-arm.cc:5834: __  mov(r2, Operand(r0, ASR, kSmiTagSize));  // y
Please remove comment // y

http://codereview.chromium.org/569015/diff/5006/4006#newcode6052
src/arm/codegen-arm.cc:6052: __ mov(r3, Operand(r1, ASR, kSmiTagSize));  // x
Use GetShiftCountLeastBits(masm, r0, r2, 5) here as well?

[zhangk, 2010-02-04 23:24:57]

http://codereview.chromium.org/569015/diff/5006/4006
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/569015/diff/5006/4006#newcode5827
src/arm/codegen-arm.cc:5827: void
GenericBinaryOpStub::GetShiftCountLeastBits(MacroAssembler* masm,
On 2010/02/04 07:38:32, Søren Gjesse wrote:

Done.

http://codereview.chromium.org/569015/diff/5006/4006#newcode5831
src/arm/codegen-arm.cc:5831: // UBFX r2, r0, #kSmiTagSize, #(5-1)
On 2010/02/04 07:38:32, Søren Gjesse wrote:

Done.

http://codereview.chromium.org/569015/diff/5006/4006#newcode5833
src/arm/codegen-arm.cc:5833: } else {
On 2010/02/04 07:38:32, Søren Gjesse wrote:

Done.

http://codereview.chromium.org/569015/diff/5006/4006#newcode5834
src/arm/codegen-arm.cc:5834: __  mov(r2, Operand(r0, ASR, kSmiTagSize));  // y
On 2010/02/04 07:38:32, Søren Gjesse wrote:

Done.

[Søren Thygesen Gjesse, 2010-02-05 07:23:12]

LGTM

http://codereview.chromium.org/569015/diff/7012/8016
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/569015/diff/7012/8016#newcode1032
src/arm/macro-assembler-arm.cc:1032: ubfx(dst, src, Operand(kSmiTagSize),
Operand(num_least_bits-1));
Spaces around binary operation.

[zhangk, 2010-02-05 21:41:56]

http://codereview.chromium.org/569015/diff/7012/8016
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/569015/diff/7012/8016#newcode1032
src/arm/macro-assembler-arm.cc:1032: ubfx(dst, src, Operand(kSmiTagSize),
Operand(num_least_bits-1));
On 2010/02/05 07:23:12, Søren Gjesse wrote:

Done.

[Søren Thygesen Gjesse, 2010-02-08 08:19:21]

On 2010/02/05 21:41:56, zhangk wrote:
> http://codereview.chromium.org/569015/diff/7012/8016
> File src/arm/macro-assembler-arm.cc (right):
> 
> http://codereview.chromium.org/569015/diff/7012/8016#newcode1032
> src/arm/macro-assembler-arm.cc:1032: ubfx(dst, src, Operand(kSmiTagSize),
> Operand(num_least_bits-1));
> On 2010/02/05 07:23:12, Søren Gjesse wrote:
> 
> Done.

Thank you for the patch. Committed as r3804.

[Søren Thygesen Gjesse, 2010-02-08 08:19:52]

On 2010/02/08 08:19:21, Søren Gjesse wrote:
> On 2010/02/05 21:41:56, zhangk wrote:
> > http://codereview.chromium.org/569015/diff/7012/8016
> > File src/arm/macro-assembler-arm.cc (right):
> > 
> > http://codereview.chromium.org/569015/diff/7012/8016#newcode1032
> > src/arm/macro-assembler-arm.cc:1032: ubfx(dst, src, Operand(kSmiTagSize),
> > Operand(num_least_bits-1));
> > On 2010/02/05 07:23:12, Søren Gjesse wrote:
> > 
> > Done.
> 
> Thank you for the patch. Committed as r3804.

Closing issue,