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#newcode2760
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#newcode2788
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#newcode2817
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#newcode2846
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#newcode2855
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#newcode245
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#newcode2465
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#newcode2508
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.
http://codereview.chromium.org/6625084/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
