ARM: Improved double to integer truncation....

ARM: Improved double to integer truncation.

Patch from ARM Ltd.

BUG=none
TEST=Added to cctest/test-assembler-arm.cc and cctest/test-disasm-arm.cc

[fschneider, 2011-03-08 14:39:23]

drive-by comment: Since the code for the truncating conversion is quite long and
we generally generate lots of conversions, I'd consider factoring it out into a
stub to reduce code size.

[Søren Thygesen Gjesse, 2011-03-08 16:15:03]

Karl, you have looked into truncation for binary operations, so could you please
do the review.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2788: // default 'round to zero' mode.
Drive-by:
How fast is the VFP rounding? Maybe just moving directly to bit-operations will
be faster.

[Karl Klose, 2011-03-08 16:40:13]

Yes, I will do the review.

[Karl Klose, 2011-03-09 10:37:51]

http://codereview.chromium.org/6625084/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/assembler-arm.h#newcode994
src/arm/assembler-arm.h:994: const Condition cond = al);
The vneg instruction should also be supported by the disassembler (see
Decoder::DecodeTypeVFP in disasm-arm.cc). Currently, the disassembler will only
print "unknown". 

Please also add a test in test-disasm-arm.cc.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-arm.h#newcode1461
src/arm/lithium-arm.h:1461: explicit LDoubleToI(LOperand* value, LOperand*
temp1, LOperand* temp2) {
explicit is not needed here.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2760: void LCodeGen::EmitECMATruncate(Register
result,
As fschneider suggested, it would be good to move this code (or parts of it) to
a stub and call it from here.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2788: // default 'round to zero' mode.
We should measure later, if bit-fiddeling code as used in IntegerConvert in
code-stubs-ia32 is actually faster.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2817: const double two_31_value = 2147483648.0;
Constants should be formatted as follows: kTwo31Value and kTwo32Value.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2846: // Add or subtrct 2^31 to easily round it
toward zero.
subtrct -> subtract.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2855: __ vcvt_s32_f64(double_input.low(),
double_input);
Should this code not use vcvt_u32_f64 as stated in the comment above?

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.h#n...
src/arm/lithium-codegen-arm.h:245: Label* done);
These functions should perhaps be in the macro assembler, because they are not
specific for lithium code generation and the macro assembler already contains FP
conversion and truncation operations.

http://codereview.chromium.org/6625084/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:2465: // The Following ARMv7 VFPv instructions are
currently supported.
Please add the vneg instruction here.

http://codereview.chromium.org/6625084/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:2508: double dd_value = - dm_value;
- dm_value -> -dm_value (no space).

[Alexandre, 2011-03-15 08:45:39]

Thanks for the review.

The code changed significatively from the previous patch. Mainly, the truncation
routine uses core registers instead of the VFP (1 fast case with the VFP still).

The new code being smaller than the previous VFP code, I did not add a stub. I
can however add one if you feel it is worth it.

Finally, this patch adds support for the vneg instruction, while it does not use
it anymore. I know we should avoid introducing new features we do not use, but
as there are cctests, support in the Simulator and Disassembler, and we will
probably use it soon, I thought I would leave it there.

Alexandre

http://codereview.chromium.org/6625084/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/assembler-arm.h#newcode994
src/arm/assembler-arm.h:994: const Condition cond = al);
On 2011/03/09 10:37:51, Karl Klose wrote:
> The vneg instruction should also be supported by the disassembler (see
> Decoder::DecodeTypeVFP in disasm-arm.cc). Currently, the disassembler will
only
> print "unknown". 
> 
> Please also add a test in test-disasm-arm.cc.

Done.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-arm.h#newcode1461
src/arm/lithium-arm.h:1461: explicit LDoubleToI(LOperand* value, LOperand*
temp1, LOperand* temp2) {
On 2011/03/09 10:37:51, Karl Klose wrote:
> explicit is not needed here.

Removed.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2760: void LCodeGen::EmitECMATruncate(Register
result,
On 2011/03/09 10:37:51, Karl Klose wrote:
> As fschneider suggested, it would be good to move this code (or parts of it)
to
> a stub and call it from here.

I first moved everything to a stub. Then  I refactored the code, which is now
smaller. I can still move it to a stub if you think it is still worth.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2788: // default 'round to zero' mode.
On 2011/03/08 16:15:03, Søren Gjesse wrote:
> Drive-by:
> How fast is the VFP rounding? Maybe just moving directly to bit-operations
will
> be faster.

I initially thought that the vfp would be faster. After some thinking and seeing
Karl's code I realized manual handling would be faster.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2817: const double two_31_value = 2147483648.0;
On 2011/03/09 10:37:51, Karl Klose wrote:
> Constants should be formatted as follows: kTwo31Value and kTwo32Value.

Done.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2846: // Add or subtrct 2^31 to easily round it
toward zero.
On 2011/03/09 10:37:51, Karl Klose wrote:
> subtrct -> subtract.

Done.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2855: __ vcvt_s32_f64(double_input.low(),
double_input);
On 2011/03/09 10:37:51, Karl Klose wrote:
> Should this code not use vcvt_u32_f64 as stated in the comment above?

No it should not. I updated the comment before to remove the ambiguity.

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/lithium-codegen-arm.h#n...
src/arm/lithium-codegen-arm.h:245: Label* done);
On 2011/03/09 10:37:51, Karl Klose wrote:
> These functions should perhaps be in the macro assembler, because they are not
> specific for lithium code generation and the macro assembler already contains
FP
> conversion and truncation operations.

Done.

http://codereview.chromium.org/6625084/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/6625084/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:2465: // The Following ARMv7 VFPv instructions are
currently supported.
On 2011/03/09 10:37:51, Karl Klose wrote:
> Please add the vneg instruction here.

Done. Also added the missing vabs.

http://codereview.chromium.org/6625084/diff/1/src/arm/simulator-arm.cc#newcod...
src/arm/simulator-arm.cc:2508: double dd_value = - dm_value;
On 2011/03/09 10:37:51, Karl Klose wrote:
> - dm_value -> -dm_value (no space).

Done.

[Karl Klose, 2011-03-15 10:18:03]

The new patch looks good to me, I am going to commit it.

I am not sure whether adding a stub would be better for the current code. I will
commit the code as is and we can add the stub later, if it is faster.

Adding the vneg operation is fine, I think, as there are a few places where we
could use it.