ARM: Improve register allocation and constraints....

ARM: Improve register allocation and constraints.
Gives ~20% boost for Crypto benchmark on A9.

BUG=none
TEST=none

Committed: http://code.google.com/p/v8/source/detail?r=8381

[Mads Ager (chromium), 2011-06-15 07:17:51]

Redirecting.

[Søren Thygesen Gjesse, 2011-06-16 09:38:07]

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

http://codereview.chromium.org/7148018/diff/1/src/arm/assembler-arm.h#newcode172
src/arm/assembler-arm.h:172: // for common values not fitting the immediate
field of vmov instructions.
It seems to be some of a waste to use 3 double registers for these constants -
they are not used that many times in the code. How about regressions on floating
point intensive benchmarks where having fewer registers could lead to more
spilling to the stack?

Also as far as I can see this change only uses 0.0 (to comvert -0.0 to 0.0 in
sqrt).

http://codereview.chromium.org/7148018/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7148018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3567: __ vstm(db_w, sp, kFirstCalleeSavedDoubleReg,
kLastCalleeSavedDoubleReg);
Oops - isn't this a bug that might have broken the C++ code calling into
JavaScript? If so this should be in a separate change so we can merge it to the
3.x branches.

Also in pre-crankshaft code we do use d8 once in a while.

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

http://codereview.chromium.org/7148018/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1122: Register scratch = scratch0();
Maybe add a comment that result might alias left (the use of Move of cause
indicate that).

http://codereview.chromium.org/7148018/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1249: void LCodeGen::DoShiftI(LShiftI* instr) {
Please add a comment that both left and right are "use at start" to indicate the
possible register aliasing.

http://codereview.chromium.org/7148018/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1303: if (shift_count == 0)  __ Move(result,
left);
I think this will be easier to read if you mode this into an else branch of the
if in the SAR and SHL case.

[Alexandre, 2011-06-16 12:58:47]

Thanks for the comments.

A separate patch for preserving the callee-saved double registers will be
uploaded.

Alexandre

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

http://codereview.chromium.org/7148018/diff/1/src/arm/assembler-arm.h#newcode172
src/arm/assembler-arm.h:172: // for common values not fitting the immediate
field of vmov instructions.
I thought we never put enough pressure on double registers to actually need 16
of them, and I have never seen such code. However I believe you are right that
using 3 registers for this purpose is useless.
I changed the code to use only one register for 0.0. -0.0 can be derived from it
with a vneg. And allocating one for 255.0 is not really useful.

This mechanism is actually used in more places via the Vmov macro, especially-
in LCodeGen::DoConstantD.
This is interesting because 0.0 is very often used as a constant.

On 2011/06/16 09:38:07, Søren Gjesse wrote:
> It seems to be some of a waste to use 3 double registers for these constants -
> they are not used that many times in the code. How about regressions on
floating
> point intensive benchmarks where having fewer registers could lead to more
> spilling to the stack?
> 
> Also as far as I can see this change only uses 0.0 (to comvert -0.0 to 0.0 in
> sqrt).

http://codereview.chromium.org/7148018/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7148018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3567: __ vstm(db_w, sp, kFirstCalleeSavedDoubleReg,
kLastCalleeSavedDoubleReg);
Indeed! I'll break it into a separate patch.
If you push the callee-saved on the stack on entry to JS and check their value
when exiting you can see they are trashed...

On 2011/06/16 09:38:07, Søren Gjesse wrote:
> Oops - isn't this a bug that might have broken the C++ code calling into
> JavaScript? If so this should be in a separate change so we can merge it to
the
> 3.x branches.
> 
> Also in pre-crankshaft code we do use d8 once in a while.

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

http://codereview.chromium.org/7148018/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1122: Register scratch = scratch0();
On 2011/06/16 09:38:07, Søren Gjesse wrote:
> Maybe add a comment that result might alias left (the use of Move of cause
> indicate that).

Done.

http://codereview.chromium.org/7148018/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1249: void LCodeGen::DoShiftI(LShiftI* instr) {
On 2011/06/16 09:38:07, Søren Gjesse wrote:
> Please add a comment that both left and right are "use at start" to indicate
the
> possible register aliasing.

Done.

http://codereview.chromium.org/7148018/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1303: if (shift_count == 0)  __ Move(result,
left);
The code for SHR was not optimal when ((shift_count == 0)&& !instr-can_deopt()).
This 'Move' is used for the three kinds of shift when (shift_count == 0), not
only for SAR and SHL. Having everything in the switch statement should still be
more readable though, so I duplicated it 3 times.
On 2011/06/16 09:38:07, Søren Gjesse wrote:
> I think this will be easier to read if you mode this into an else branch of
the
> if in the SAR and SHL case.

[Søren Thygesen Gjesse, 2011-06-22 07:05:48]

LGTM, but the patch needs to be rebased to current head. It does not currently
apply, as parts of this is now committed separately in r8357.

error: patch failed: src/arm/assembler-arm.h:303
error: src/arm/assembler-arm.h: patch does not apply
error: patch failed: src/arm/code-stubs-arm.cc:3550
error: src/arm/code-stubs-arm.cc: patch does not apply
error: patch failed: src/arm/frames-arm.h:72
error: src/arm/frames-arm.h: patch does not apply
Failed to apply the patch

[Søren Thygesen Gjesse, 2011-06-22 19:51:21]

On 2011/06/22 10:57:43, m.m.capewell wrote:

Committed. Thank you for the patch.