ARM: Make use of d16-d31 when available.

ARM: Make use of d16-d31 when available.

Committed: https://code.google.com/p/v8/source/detail?r=13484

[hans, 2012-12-03 14:54:09]

This is a work-in-progress patch, not ready for proper review yet. Just
uploading to get some early comments and in case my hard drive fails :)

Things that are missing:

 - This doesn't pass the test suite, something is broken
 - Style issues
 - Doesn't detect number of registers at run-time
 - Want https://chromiumcodereview.appspot.com/11191029/ to solve VMOD imm
problem
 - Needs to re-base on and re-use stuff in
https://codereview.chromium.org/10701054/

[Rodolph Perfetta, 2012-12-04 14:08:40]

Just a few comments for the arm specific files

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

https://codereview.chromium.org/11428137/diff/1/src/arm/assembler-arm.h#newco...
src/arm/assembler-arm.h:207: return from_code(NumAvailableRegisters() - 1);
last callee saved register is d15 even if you have 32d registers.

https://codereview.chromium.org/11428137/diff/1/src/arm/assembler-arm.h#newco...
src/arm/assembler-arm.h:230: if (index > ZeroReg().code())
>= otherwise index 14 maps to d14 which is zero.

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

https://codereview.chromium.org/11428137/diff/1/src/arm/lithium-arm.h#newcode...
src/arm/lithium-arm.h:2338: LOperand*
double_register_spills_[DoubleRegister::kNumRegisters];
Why not NumAllocatableRegisters()?

[hans, 2012-12-04 14:30:54]

Thanks for the comments Rodolph! New patch uploaded.

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

https://codereview.chromium.org/11428137/diff/1/src/arm/assembler-arm.h#newco...
src/arm/assembler-arm.h:207: return from_code(NumAvailableRegisters() - 1);
On 2012/12/04 14:08:40, Rodolph Perfetta wrote:
> last callee saved register is d15 even if you have 32d registers.

Done.

https://codereview.chromium.org/11428137/diff/1/src/arm/assembler-arm.h#newco...
src/arm/assembler-arm.h:230: if (index > ZeroReg().code())
On 2012/12/04 14:08:40, Rodolph Perfetta wrote:
> >= otherwise index 14 maps to d14 which is zero.

oops. done.

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

https://codereview.chromium.org/11428137/diff/1/src/arm/lithium-arm.h#newcode...
src/arm/lithium-arm.h:2338: LOperand*
double_register_spills_[DoubleRegister::kNumRegisters];
On 2012/12/04 14:08:40, Rodolph Perfetta wrote:
> Why not NumAllocatableRegisters()?

Because the array size has to be a constant.

This means wasting some space when only 16 registers are available, but I don't
think that will make any difference.

[Rodolph Perfetta, 2012-12-05 00:42:10]

https://codereview.chromium.org/11428137/diff/5001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/5001/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2030: // Push r4, r5 on the stack.
There is an instruction which lets you move an ARM register into the bottom or
top word of any D register: vmov.32. For example vmov.32 d16[0], ip will move ip
in the lower 32bits of d16. It can be found section A8.6.328 of the ARM ARM
(move core reg to scalar)

It is not implemented in V8 though and note that vmov.16 and vmov.8 are NEON
only instructions.

https://codereview.chromium.org/11428137/diff/5001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/11428137/diff/5001/src/arm/code-stubs-arm.h#n...
src/arm/code-stubs-arm.h:477: // XXX: Why is d0 special? It's not the scratch
register..
A long time ago, d0 was the scratch register, so this may be a remain from this
time. This method is currently saving callee saved register too.

https://codereview.chromium.org/11428137/diff/5001/src/arm/code-stubs-arm.h#n...
src/arm/code-stubs-arm.h:490: // XXX: Why is d0 special? It's not the scratch
register..
Ditto.

[hans, 2012-12-05 14:12:58]

https://codereview.chromium.org/11428137/diff/5001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/5001/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2030: // Push r4, r5 on the stack.
On 2012/12/05 00:42:10, Rodolph Perfetta wrote:
> There is an instruction which lets you move an ARM register into the bottom or
> top word of any D register: vmov.32. For example vmov.32 d16[0], ip will move
ip
> in the lower 32bits of d16. It can be found section A8.6.328 of the ARM ARM
> (move core reg to scalar)
> 
> It is not implemented in V8 though and note that vmov.16 and vmov.8 are NEON
> only instructions.

Awesome, I'll just use vmov.32 then. Thanks!

https://codereview.chromium.org/11428137/diff/5001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/11428137/diff/5001/src/arm/code-stubs-arm.h#n...
src/arm/code-stubs-arm.h:477: // XXX: Why is d0 special? It's not the scratch
register..
On 2012/12/05 00:42:10, Rodolph Perfetta wrote:
> A long time ago, d0 was the scratch register, so this may be a remain from
this
> time. This method is currently saving callee saved register too.

So I guess what we want here is to save d0--d7, and d16--d31 when available?
(Same below)

[Rodolph Perfetta, 2012-12-05 21:39:34]

https://codereview.chromium.org/11428137/diff/12002/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/11428137/diff/12002/src/arm/code-stubs-arm.h#...
src/arm/code-stubs-arm.h:477: // XXX: Why is d0 special? It's not the scratch
register..
To reply to your previous question, yes only d0-d7, d16-d31 should be saved. I
had a brief look and I couldn't find where d0 is saved separately, so it may
well be a bug in the existing code.

https://codereview.chromium.org/11428137/diff/12002/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/12002/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:852: vldm(am, base, DwVfpRegister::from_code(16),
last, cond);
Wouldn't d16 be more readable here? Same below

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1059: __ sub(fp, ip, Operand(4));
Why is this necessary? the tests don't seem to use fp?

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1168: __ sub(fp, ip, Operand(4));
fp is not used.

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1170: // Load a, b, c into d16, d17, d18.
The values loaded do not get used. Is this intentional?

[hans, 2012-12-11 17:08:04]

Thanks for the comments, and thank you very much for the help with debugging the
OSR issue.

The new patch passes all tests both in simulator and on device, and can also run
through the whole test suite.

It's still missing run-time detection of this feature, but otherwise this is
getting close to completeness.

https://codereview.chromium.org/11428137/diff/12002/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/11428137/diff/12002/src/arm/code-stubs-arm.h#...
src/arm/code-stubs-arm.h:477: // XXX: Why is d0 special? It's not the scratch
register..
On 2012/12/05 21:39:34, Rodolph Perfetta wrote:
> To reply to your previous question, yes only d0-d7, d16-d31 should be saved. I
> had a brief look and I couldn't find where d0 is saved separately, so it may
> well be a bug in the existing code.

I'll do a fix in a separate patch.

https://codereview.chromium.org/11428137/diff/12002/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/12002/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:852: vldm(am, base, DwVfpRegister::from_code(16),
last, cond);
On 2012/12/05 21:39:34, Rodolph Perfetta wrote:
> Wouldn't d16 be more readable here? Same below

Done.

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1059: __ sub(fp, ip, Operand(4));
On 2012/12/05 21:39:34, Rodolph Perfetta wrote:
> Why is this necessary? the tests don't seem to use fp?

You're right, I was copy-pasting too aggressively :) Removed.

https://codereview.chromium.org/11428137/diff/12002/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1168: __ sub(fp, ip, Operand(4));
On 2012/12/05 21:39:34, Rodolph Perfetta wrote:
> fp is not used.

I didn't mean to upload this test. I've updated test 13 to cover everything
instead, so this is obsolete.

[Rodolph Perfetta, 2012-12-12 14:29:15]

https://codereview.chromium.org/11428137/diff/21001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1692: ASSERT(offset >= 0);
There is the same assert two lines below.

https://codereview.chromium.org/11428137/diff/21001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2022: if (dst.code() < 16) {
vmov.32 could also be used when dst.code() < 16. the code would be simpler.

https://codereview.chromium.org/11428137/diff/21001/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:1040: __ mov(r4, r0);
minor tweak: the code below could be rewritten not to corrupt r0, removing the
need for this mov.

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:834: void MacroAssembler::Vldm(BlockAddrMode am,
Do you intend to support all addressing modes for this macro instruction? If not
how about renaming it to Vpush or something similar?

Same for Vldm.

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:843: const int first_code = first.code();
const should start with a k

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:867: const int first_code = first.code();
const should start with a k.

https://codereview.chromium.org/11428137/diff/21001/test/cctest/test-assemble...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1057: __ mov(ip, Operand(sp));
This is not needed/used.

[hans, 2012-12-12 17:58:47]

Thanks for the comments! I've addressed them below and uploaded a new patch.


A new interesting issue has come up: snapshots. They have to work across
devices, so can only use a minimal set of features, i.e. only 16 d-regs, but
then that snapshot can later be used on a device with 32 d-regs.

Most places that depend on the number of available registers aren't run as part
of the snapshot generation, so I'll put asserts there. One function does run,
though:

Deoptimizer::EntryGenerator::Generate()

And I think it would be bad to have that one generate code assuming 16 d-regs,
and then executed later with 32 r-regs :/

https://codereview.chromium.org/11428137/diff/21001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1692: ASSERT(offset >= 0);
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> There is the same assert two lines below.

Removed.

https://codereview.chromium.org/11428137/diff/21001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2022: if (dst.code() < 16) {
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> vmov.32 could also be used when dst.code() < 16. the code would be simpler.

Done.

https://codereview.chromium.org/11428137/diff/21001/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:1040: __ mov(r4, r0);
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> minor tweak: the code below could be rewritten not to corrupt r0, removing the
> need for this mov.

Is it OK if I note this in a TODO instead, so as to keep the diff small? This
will be easy to fix in a separate patch.

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:834: void MacroAssembler::Vldm(BlockAddrMode am,
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> Do you intend to support all addressing modes for this macro instruction? If
not
> how about renaming it to Vpush or something similar?

Yes, that sounds like a good idea. Done.

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:843: const int first_code = first.code();
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> const should start with a k

I think that's for compile-time constants only.

https://codereview.chromium.org/11428137/diff/21001/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:867: const int first_code = first.code();
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> const should start with a k.

Ditto.

https://codereview.chromium.org/11428137/diff/21001/test/cctest/test-assemble...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/21001/test/cctest/test-assemble...
test/cctest/test-assembler-arm.cc:1057: __ mov(ip, Operand(sp));
On 2012/12/12 14:29:15, Rodolph Perfetta wrote:
> This is not needed/used.

Removed.

[hans, 2012-12-14 15:36:20]

On 2012/12/12 17:58:47, hans wrote:
> A new interesting issue has come up: snapshots. They have to work across
> devices, so can only use a minimal set of features, i.e. only 16 d-regs, but
> then that snapshot can later be used on a device with 32 d-regs.
> 
> Most places that depend on the number of available registers aren't run as
part
> of the snapshot generation, so I'll put asserts there. One function does run,
> though:
> 
> Deoptimizer::EntryGenerator::Generate()
> 
> And I think it would be bad to have that one generate code assuming 16 d-regs,
> and then executed later with 32 r-regs :/

I have uploaded a new version of the patch which solves this problem by not
including that builtin in snapshots on ARM. I would be grateful for some careful
review on this.

[ulan, 2012-12-17 10:44:52]

Some nits and comments:

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
File src/arm/assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:318: if (index >= 14)
Here and below we can use kDoubleRegZero.code() instead of 14.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:1111: int x,
I wonder if there is more descriptive name than 'x'.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
File src/arm/code-stubs-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
src/arm/code-stubs-arm.h:472: // Nbr of d-regs not known at snapshot time.
Nbr => Number

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/isolate.cc#new...
src/isolate.cc:2029: builtins_.SetUp(false, true);
This doesn't cover the case when snapshot is completely disabled (can be tested
with "make snapshot=off" command). In this case create_heap_objects will be
true, but Serializer()::enabled() will be false, and we will miss generating
non-snapshot builtins.

What if we combine the two flag arguments of the SetUp() into an enum with
descriptive names, e.g. (maybe there are better names):
GENERATE_FOR_SNAPSHOT, GENERATE_ALL, GENERATE_BEFORE_DESERIALIZATION,
GENERATE_AFTER_DESERIALIZATION.

Then we can call SetUp with the correct argument depending on
create_heap_objects, Serializer()::enabled().

[hans, 2012-12-17 12:31:43]

Thanks very much for the comments! New patch uploaded.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
File src/arm/assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:318: if (index >= 14)
On 2012/12/17 10:44:52, ulan wrote:
> Here and below we can use kDoubleRegZero.code() instead of 14.

I don't think that would work, since kDoubleRegZero hasn't been defined at this
point, and it expands to d14 which also hasn't been defined yet (and can't be
defined before this struct).

I guess I could just declare FromAllocationIndex here, and then put the
definition further below, when d14 and kDoubleRegZero are available. Would you
prefer that?

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:1111: int x,
On 2012/12/17 10:44:52, ulan wrote:
> I wonder if there is more descriptive name than 'x'.

How about "index"?

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
File src/arm/code-stubs-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
src/arm/code-stubs-arm.h:472: // Nbr of d-regs not known at snapshot time.
On 2012/12/17 10:44:52, ulan wrote:
> Nbr => Number

Done.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/isolate.cc#new...
src/isolate.cc:2029: builtins_.SetUp(false, true);
On 2012/12/17 10:44:52, ulan wrote:
> This doesn't cover the case when snapshot is completely disabled (can be
tested
> with "make snapshot=off" command). In this case create_heap_objects will be
> true, but Serializer()::enabled() will be false, and we will miss generating
> non-snapshot builtins.
> 
> What if we combine the two flag arguments of the SetUp() into an enum with
> descriptive names, e.g. (maybe there are better names):
> GENERATE_FOR_SNAPSHOT, GENERATE_ALL, GENERATE_BEFORE_DESERIALIZATION,
> GENERATE_AFTER_DESERIALIZATION.
> 
> Then we can call SetUp with the correct argument depending on
> create_heap_objects, Serializer()::enabled().

That sounds like a good idea.

[ulan, 2012-12-17 13:03:36]

Thank you, Hans! The CL overall looks good to me. I am reviewing and verifying
small details, like instruction encoding changes.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
File src/arm/assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:318: if (index >= 14)
I see. Yep, putting FromAllocationIndex near the ToAllocationIndex would make it
more readable.

On 2012/12/17 12:31:44, hans wrote:
> On 2012/12/17 10:44:52, ulan wrote:
> > Here and below we can use kDoubleRegZero.code() instead of 14.
> 
> I don't think that would work, since kDoubleRegZero hasn't been defined at
this
> point, and it expands to d14 which also hasn't been defined yet (and can't be
> defined before this struct).
> 
> I guess I could just declare FromAllocationIndex here, and then put the
> definition further below, when d14 and kDoubleRegZero are available. Would you
> prefer that?

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:1111: int x,
On 2012/12/17 12:31:44, hans wrote:
> On 2012/12/17 10:44:52, ulan wrote:
> > I wonder if there is more descriptive name than 'x'.
> 
> How about "index"?
Yep, I like it more.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
File src/arm/code-stubs-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
src/arm/code-stubs-arm.h:472: // Nbr of d-regs not known at snapshot time.
Thanks, there few other places with the same comment.
On 2012/12/17 12:31:44, hans wrote:
> On 2012/12/17 10:44:52, ulan wrote:
> > Nbr => Number
> 
> Done.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/isolate.cc#new...
src/isolate.cc:2029: builtins_.SetUp(false, true);
Thanks, this part looks good now.
On 2012/12/17 12:31:44, hans wrote:
> On 2012/12/17 10:44:52, ulan wrote:
> > This doesn't cover the case when snapshot is completely disabled (can be
> tested
> > with "make snapshot=off" command). In this case create_heap_objects will be
> > true, but Serializer()::enabled() will be false, and we will miss generating
> > non-snapshot builtins.
> > 
> > What if we combine the two flag arguments of the SetUp() into an enum with
> > descriptive names, e.g. (maybe there are better names):
> > GENERATE_FOR_SNAPSHOT, GENERATE_ALL, GENERATE_BEFORE_DESERIALIZATION,
> > GENERATE_AFTER_DESERIALIZATION.
> > 
> > Then we can call SetUp with the correct argument depending on
> > create_heap_objects, Serializer()::enabled().
> 
> That sounds like a good idea.

[hans, 2012-12-17 13:53:21]

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
File src/arm/assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:318: if (index >= 14)
On 2012/12/17 13:03:36, ulan wrote:
> I see. Yep, putting FromAllocationIndex near the ToAllocationIndex would make
it
> more readable.

Cool. I've moved it and AllocationIndexToString.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/assembler-...
src/arm/assembler-arm.h:1111: int x,
On 2012/12/17 13:03:36, ulan wrote:
> On 2012/12/17 12:31:44, hans wrote:
> > On 2012/12/17 10:44:52, ulan wrote:
> > > I wonder if there is more descriptive name than 'x'.
> > 
> > How about "index"?
> Yep, I like it more.

Done.

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
File src/arm/code-stubs-arm.h (right):

https://chromiumcodereview.appspot.com/11428137/diff/38001/src/arm/code-stubs...
src/arm/code-stubs-arm.h:472: // Nbr of d-regs not known at snapshot time.
On 2012/12/17 13:03:36, ulan wrote:
> Thanks, there few other places with the same comment.

I've changed them all now.

[Rodolph Perfetta, 2012-12-17 14:38:30]

https://codereview.chromium.org/11428137/diff/40002/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/11428137/diff/40002/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:61: if (index >= kDoubleRegZero.code())
Do we need to assert kScratchDoubleReg.code() - kDoubleRegZero.code() ==
kNumReservedRegiters - 1 ?

Same below.

https://codereview.chromium.org/11428137/diff/40002/src/platform-linux.cc
File src/platform-linux.cc (right):

https://codereview.chromium.org/11428137/diff/40002/src/platform-linux.cc#new...
src/platform-linux.cc:160: search_string = "vfpv3";
The search_string variable is not used with VFP32DREGS since you have singled
out the case VFP32DREGS below.

https://codereview.chromium.org/11428137/diff/40002/src/platform-linux.cc#new...
src/platform-linux.cc:167: return CPUInfoContainsString("vfpv3") &&
!CPUInfoContainsString("d16");
I believe some early devices such as nexus one do not report vfpv3 but vfp (see
special case feature == VFP3 below).

Maybe something on the line of

if (ArmCpuHasFeature(VFP3) && !CPUInfoContainsString("d16"))

would work better.

[hans, 2012-12-17 15:32:45]

Thanks, Rodolph! New patch uploaded.

https://codereview.chromium.org/11428137/diff/40002/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/11428137/diff/40002/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:61: if (index >= kDoubleRegZero.code())
On 2012/12/17 14:38:30, Rodolph Perfetta wrote:
> Do we need to assert kScratchDoubleReg.code() - kDoubleRegZero.code() ==
> kNumReservedRegiters - 1 ?
> 
> Same below.

Good idea! Done.

https://codereview.chromium.org/11428137/diff/40002/src/platform-linux.cc
File src/platform-linux.cc (right):

https://codereview.chromium.org/11428137/diff/40002/src/platform-linux.cc#new...
src/platform-linux.cc:160: search_string = "vfpv3";
On 2012/12/17 14:38:30, Rodolph Perfetta wrote:
> The search_string variable is not used with VFP32DREGS since you have singled
> out the case VFP32DREGS below.

Done.

https://codereview.chromium.org/11428137/diff/40002/src/platform-linux.cc#new...
src/platform-linux.cc:167: return CPUInfoContainsString("vfpv3") &&
!CPUInfoContainsString("d16");
On 2012/12/17 14:38:30, Rodolph Perfetta wrote:
> I believe some early devices such as nexus one do not report vfpv3 but vfp
(see
> special case feature == VFP3 below).
> 
> Maybe something on the line of
> 
> if (ArmCpuHasFeature(VFP3) && !CPUInfoContainsString("d16"))
> 
> would work better.

Done.

[Rodolph Perfetta, 2012-12-17 15:37:21]

https://codereview.chromium.org/11428137/diff/50003/src/platform-linux.cc
File src/platform-linux.cc (right):

https://codereview.chromium.org/11428137/diff/50003/src/platform-linux.cc#new...
src/platform-linux.cc:167: return ArmCpuHasFeature(ARMv3) &&
!CPUInfoContainsString("d16");
Typo: ARMv3 -> VFP3

[hans, 2012-12-17 15:49:15]

https://codereview.chromium.org/11428137/diff/50003/src/platform-linux.cc
File src/platform-linux.cc (right):

https://codereview.chromium.org/11428137/diff/50003/src/platform-linux.cc#new...
src/platform-linux.cc:167: return ArmCpuHasFeature(ARMv3) &&
!CPUInfoContainsString("d16");
On 2012/12/17 15:37:21, Rodolph Perfetta wrote:
> Typo: ARMv3 -> VFP3

Mondays.. Thanks for catching this :)

[Rodolph Perfetta, 2012-12-17 20:03:25]

https://codereview.chromium.org/11428137/diff/39003/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/11428137/diff/39003/src/flag-definitions.h#ne...
src/flag-definitions.h:301: DEFINE_bool(enable_32dregs, false,
I would set this to true by default and update CpuFeatures::IsSupported to be
overridden by the flag when it is false (in the same way as VFP3).

currently the flag only seems to affect the simulator:
 * on the command line if you set the flag to true but the hardware doesn't
support it, it will be ignored.
 * when the flag is false but the hardware provides 32 D-regs then the feature
is enabled by runtime probing.

If you default to true and update IsSupported then you can disabled the feature
even if the CPU supports it.

[hans, 2012-12-18 11:09:08]

https://codereview.chromium.org/11428137/diff/39003/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/11428137/diff/39003/src/flag-definitions.h#ne...
src/flag-definitions.h:301: DEFINE_bool(enable_32dregs, false,
On 2012/12/17 20:03:25, Rodolph Perfetta wrote:
> I would set this to true by default and update CpuFeatures::IsSupported to be
> overridden by the flag when it is false (in the same way as VFP3).
> 
> currently the flag only seems to affect the simulator:
>  * on the command line if you set the flag to true but the hardware doesn't
> support it, it will be ignored.
>  * when the flag is false but the hardware provides 32 D-regs then the feature
> is enabled by runtime probing.
> 
> If you default to true and update IsSupported then you can disabled the
feature
> even if the CPU supports it.

I was worried that this would get included in the snapshot if enabled by
default, but after talking to Ulan we concluded that this doesn't happen because
CpuFeatures::Probe() returns early in the case that the serializer is enabled.

Also added a compile-time flag that can be used to enable this feature both for
the snapshot and target.

[hans, 2012-12-19 13:48:32]

A new problem has come up: https://code.google.com/p/v8/source/detail?r=13236
I've uploaded an attempt to rebase on top of that, but there are some problems:

- Snapshot generation for IA32 currently fails with: 

    Fatal error in ../src/lithium-allocator.cc, line 1918
    CHECK(current->Start().Value() < register_use->pos().Value()) failed

  I'm not sure how I broke that, but it's hopefully some simple mistake
somewhere.


- Snapshot generation now includes more stuff that depends on the number of
registers :(
  I've had to disable a number of the ASSERT(!Serializer::enabled()), because
they don't hold anymore.
  I think we might need a new strategy here.. before we were lucky that it was
just OnStackReplacement
  that depended on the number of registers and got included in the snapshot. It
seems that luck has run out.

[hans, 2013-01-10 14:32:23]

I've rebased and uploaded the patch again.

I fixed the problem that caused an assert in lithium-allocator.cc.

Also, I removed the code that was used to keep the OnStackReplacement builtin
out of snapshots; this technique doesn't work after r13236, because there is now
more code in the snapshot that depends on the number of registers. The following
functions that depend on the number of registers now get hit during snapshot
creation:

 - StoreBufferOverflowStub::Generate
 - RecordWriteStub::RegisterAllocation::SaveCallerSaveRegisters
 - RecordWriteStub::RegisterAllocation::RestoreCallerSaveRegisters
 - Deoptimizer::EntryGenerator::Generate
 - MacroAssembler::EnterFrame
 - MacroAssembler::LeaveExitFrame

We still need a way to fix that.


What about performance? I added a printf in LAllocator::Spill and counted the
number of times a double gets spilled. In Octane, the number of spills does go
down for some benchmarks (e.g. in Box2D) when using d16-d31, but I couldn't
measure any performance gain.

In Kraken, there are again some benchmarks that get less spills with my patch. I
couldn't see any overall performance improvement. Focusing on
imaging-darkroom.js, I measured a 0.7% speed-up (Nexus 4, 5 runs, 20 s per run),
but that could also be noise.

I also wrote a matmul benchmark (below). This gets a 12% speed-up with my patch
(Nexys 4, 5 runs).

function matmul(left, right) {
  var a11 = left[ 0]; var a12 = left[ 1]; var a13 = left[ 2]; var a14 = left[
3];
  var a21 = left[ 4]; var a22 = left[ 5]; var a23 = left[ 6]; var a24 = left[
7];
  var a31 = left[ 8]; var a32 = left[ 9]; var a33 = left[10]; var a34 =
left[11];
  var a41 = left[12]; var a42 = left[13]; var a43 = left[14]; var a44 =
left[15];

  var b11 = right[ 0]; var b12 = right[ 1]; var b13 = right[ 2]; var b14 =
right[ 3];
  var b21 = right[ 4]; var b22 = right[ 5]; var b23 = right[ 6]; var b24 =
right[ 7];
  var b31 = right[ 8]; var b32 = right[ 9]; var b33 = right[10]; var b34 =
right[11];
  var b41 = right[12]; var b42 = right[13]; var b43 = right[14]; var b44 =
right[15];

  var c = new Array(16);

  for (var i = 0; i < 100000000; ++i) {
    // c11 .. c14
    c[ 0] = a11 * b11 + a12 * b21 + a13 * b31 + a14 * b41;
    c[ 1] = a11 * b12 + a12 * b22 + a13 * b32 + a14 * b42;
    c[ 2] = a11 * b13 + a12 * b23 + a13 * b33 + a14 * b43;
    c[ 3] = a11 * b14 + a12 * b24 + a13 * b34 + a14 * b44;

    // c21 .. c24
    c[ 4] = a21 * b11 + a22 * b21 + a23 * b31 + a24 * b41;
    c[ 5] = a21 * b12 + a22 * b22 + a23 * b32 + a24 * b42;
    c[ 6] = a21 * b13 + a22 * b23 + a23 * b33 + a24 * b43;
    c[ 7] = a21 * b14 + a22 * b24 + a23 * b34 + a24 * b44;

    // c31 .. c34
    c[ 8] = a31 * b11 + a32 * b21 + a33 * b31 * a34 * b41;
    c[ 9] = a31 * b12 + a32 * b22 + a33 * b32 * a34 * b42;
    c[10] = a31 * b13 + a32 * b23 + a33 * b33 * a34 * b43;
    c[11] = a31 * b14 + a32 * b24 + a33 * b34 * a34 * b44;

    // c41 .. c44
    c[12] = a41 * b11 + a42 * b21 + a43 * b31 + a44 * b41;
    c[13] = a41 * b12 + a42 * b22 + a43 * b32 + a44 * b42;
    c[14] = a41 * b13 + a42 * b23 + a43 * b33 + a44 * b43;
    c[15] = a41 * b14 + a42 * b24 + a43 * b34 + a44 * b44;
  }

  return c;
}

function test() {
  var a = [1.1, 2.1, 3.1, 4.1, 5.1, 6.1, 7.1, 8.1, 9.1, 10.1, 11.1, 12.1, 13.1,
14.1, 15.1, 16.1];
  var b = [1.2, 2.2, 3.2, 4.2, 5.2, 6.2, 7.2, 8.2, 9.2, 10.2, 11.2, 12.2, 13.2,
14.2, 15.2, 16.2];

  c = matmul(a, b);
  print(c);
}

test();

[hans, 2013-01-17 15:07:50]

I've uploaded a new version of the patch that solves the problem with snapshots.

Code that is generated at snapshot generation time and depends on the number of
registers now uses an ExternalReference to look at the CpuFeatures bits and do
the right thing at run-time.

Please take a look.

[hans, 2013-01-17 16:35:18]

I moved the common code for checking the feature flag into
MacroAssembler::CheckFor32DRegs as suggested by Ulan.

[ulan_google, 2013-01-18 10:44:42]

LGTM

https://codereview.chromium.org/11428137/diff/84001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/11428137/diff/84001/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:80: if (reg.code() > kDoubleRegZero.code())
Nit: brackets are needed around "if" body.

https://codereview.chromium.org/11428137/diff/84001/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:90: if (index >= kDoubleRegZero.code())
Nit: brackets are needed around "if" body.

[Rodolph Perfetta, 2013-01-18 21:36:52]

https://codereview.chromium.org/11428137/diff/85032/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

https://codereview.chromium.org/11428137/diff/85032/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:993: __ add(sp, sp, Operand(16 * kDoubleSize),
LeaveCC, eq);
You probably meant sub, with add you are overwriting the stack.

Why do you always make space for 32 values on the stack?

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:891: sub(sp, sp, Operand(16 * kDoubleSize),
LeaveCC, eq);
I don't see why this is necessary, if it is maybe a comment would be good.

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:3406: void
MacroAssembler::CheckFor32DRegs(Register scratch) {
The scratch parameter is nerver used, remove.

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:3409: and_(ip, ip, Operand(1u << VFP32DREGS),
SetCC);
Since you don't use the result of the and use tst instead.

[hans, 2013-01-22 13:55:40]

I've uploaded a new patch that addresses your comments.

Also, I have changed Assembler::vmov(dst, imm, ...) to only use vmov.32 for d16
and higher. It turns out vmov.32 ends up being slower than moving to
s-registers, causing the matmul benchmark above to regress on some devices.

The current patch gives matmul a speed-up on every device I've tested.

https://codereview.chromium.org/11428137/diff/84001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/11428137/diff/84001/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:80: if (reg.code() > kDoubleRegZero.code())
On 2013/01/18 10:44:42, ulan1 wrote:
> Nit: brackets are needed around "if" body.

Done.

https://codereview.chromium.org/11428137/diff/84001/src/arm/assembler-arm-inl...
src/arm/assembler-arm-inl.h:90: if (index >= kDoubleRegZero.code())
On 2013/01/18 10:44:42, ulan1 wrote:
> Nit: brackets are needed around "if" body.

Done.

https://codereview.chromium.org/11428137/diff/85032/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

https://codereview.chromium.org/11428137/diff/85032/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:993: __ add(sp, sp, Operand(16 * kDoubleSize),
LeaveCC, eq);
On 2013/01/18 21:36:52, Rodolph Perfetta wrote:
> You probably meant sub, with add you are overwriting the stack.
Yes! I noticed this made everything crash reliably on d16 devices :)

> Why do you always make space for 32 values on the stack?
There is an assert somewhere that checks the size of stuff on the stack. It
seemed safest to use the same stack layout in both the 16 and 32 register cases.

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:891: sub(sp, sp, Operand(16 * kDoubleSize),
LeaveCC, eq);
On 2013/01/18 21:36:52, Rodolph Perfetta wrote:
> I don't see why this is necessary, if it is maybe a comment would be good.

It's because otherwise the comment below wouldn't hold, since
DwVfpRegisters::kNumRegisters is always 32.

Maybe it would be possible to use different stack layout depending on number of
registers, but it seemed safer to always keep it the same.

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:3406: void
MacroAssembler::CheckFor32DRegs(Register scratch) {
On 2013/01/18 21:36:52, Rodolph Perfetta wrote:
> The scratch parameter is nerver used, remove.

Oops, the intention was to use scratch below instead of ip, in case the caller
is using ip for something else.

https://codereview.chromium.org/11428137/diff/85032/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:3409: and_(ip, ip, Operand(1u << VFP32DREGS),
SetCC);
On 2013/01/18 21:36:52, Rodolph Perfetta wrote:
> Since you don't use the result of the and use tst instead.
Done.

[ulan, 2013-01-23 10:46:18]

New patch set looks good to me, we can land after getting LGTM from Rodolph.

[Rodolph Perfetta, 2013-01-23 11:51:55]

lgtm