Use generated Neon version of MemCopy() on ARM, if platform supports it.

Use generated Neon version of MemCopy() on ARM, if platform supports it.
Neon detection is implemented by reading /proc/cpuinfo.
Extend ARM assembler with few multiple load/multiple store
Neon-specific instructions.

[Nike, 2013-03-26 12:42:10]

Neon MemCopy().

[danno, 2013-03-27 08:40:07]

Thanks for the patch. This is going in the right direction, there are quite a
few styling nits, though. Please see my comments.

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

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2667: 
nit: coding style, 2 empty lines between method definitions, here and below

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2706: enum NeonElementType {
Why is this not in assembler-arm.h?

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2711: enum NeonLoadStoreType {
same here?

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

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.h#ne...
src/arm/assembler-arm.h:457: };
nit: add a empty line after this enum

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.h#ne...
src/arm/assembler-arm.h:1144: const Register base,
nit: parameter indentation here and below, please follow Google C++ coding
style.

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (left):

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc#old...
src/arm/codegen-arm.cc:120: 
Please remove this extraneous whitespace change.

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc#new...
src/arm/codegen-arm.cc:137: fix_remainder, main_loop, has32, less32;
coding style: 4-char indent on line continuation.

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc#new...
src/arm/codegen-arm.cc:153: __ b(&less16, lo);
Note that the per-OS generated mem copy doesn't need to support copies of less
than a specific size (OS::kMinComplexMemCopy). You should take that into account
here (i.e. not handle the smaller sizes at all) to save complexity of this code.
Feel free to add a debug-only assert that detects copy sizes that are too small.

[Jakob Kummerow, 2013-03-27 22:51:34]

What happened to "we don't use the NEON unit so that it can stay powered down
and save some power"?

[danno, 2013-03-27 22:57:03]

It's my understanding that SVG uses NEON all the time, so it's already powered
up anyway.

[hans, 2013-03-28 14:39:26]

On 2013/03/27 22:57:03, danno wrote:
> It's my understanding that SVG uses NEON all the time, so it's already powered
> up anyway.

Shouldn't the platform's libc (bionic in the case of Android) already provide a
NEON-based memcpy if the hardware supports it?

For bionic, it seems to be here:
https://android.googlesource.com/platform/bionic/+/jb-mr1.1-release/libc/arch...

Does V8 gain anything by having its own implementation?

[Nike, 2013-03-29 08:21:51]

I see few points, why runtime generation of MemCopy() copy could be desirable.
 - we can use runtime info, such as cache size, for better tuning of the
implementation on the fly (see kCacheLineSize and kPrefetchDistance). For now
they are hardcoded, but it's easy to change. This doc:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0438c/BABHAEIF...
suggest that Cortex has 64 byte cacheline, while others may have 32 byte
cacheline.
 - this fast implementation could be used as long as we find Neon, even on
systems with slower system memcpy
 - minor advantage is that as MemCopy() has different signature that memcpy()
(no return value), we can save/restore one register less
 - also it adds Neon detection and few Neon instruction to assembler, which
could be generally useful
 - we could create MemCopy() versions with more preconditions, such as word
aligned copy, out of the same generic code

[Nike, 2013-03-29 08:48:52]

PTAL

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

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2667: 
On 2013/03/27 08:40:07, danno wrote:
> nit: coding style, 2 empty lines between method definitions, here and below

Done.

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2706: enum NeonElementType {
Because those enums are never used by user, only as parameters to generic
function creating vector instructions.

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:2711: enum NeonLoadStoreType {
Same as above.

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

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.h#ne...
src/arm/assembler-arm.h:457: };
On 2013/03/27 08:40:07, danno wrote:
> nit: add a empty line after this enum

Done.

https://codereview.chromium.org/12920009/diff/3002/src/arm/assembler-arm.h#ne...
src/arm/assembler-arm.h:1144: const Register base,
On 2013/03/27 08:40:07, danno wrote:
> nit: parameter indentation here and below, please follow Google C++ coding
> style. 

Done.

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc#new...
src/arm/codegen-arm.cc:137: fix_remainder, main_loop, has32, less32;
On 2013/03/27 08:40:07, danno wrote:
> coding style: 4-char indent on line continuation.

Done.

https://codereview.chromium.org/12920009/diff/3002/src/arm/codegen-arm.cc#new...
src/arm/codegen-arm.cc:153: __ b(&less16, lo);
On 2013/03/27 08:40:07, danno wrote:
> Note that the per-OS generated mem copy doesn't need to support copies of less
> than a specific size (OS::kMinComplexMemCopy). You should take that into
account
> here (i.e. not handle the smaller sizes at all) to save complexity of this
code.
> Feel free to add a debug-only assert that detects copy sizes that are too
small.

Done.

[hans, 2013-04-02 12:34:29]

I've got some style nits mostly. I still haven't penetrated the actual copy
routine or the assembler for the NEON instructions.

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

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1605: void Assembler::pld(Register base, int offset,
int write) {
Many of the newly added functions to the assembler have references to the
appropriate section in the ARM ARM, and also a comment that lays out the
instruction encoding that makes it much easier to verify that the code is
correct. See e.g. vadd below.

All new assembler functions also need corresponding changes in the simulator and
disassembler + tests.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1606: int U = (offset >= 0) ? B23 : 0;
looks like indentation is too deep in this function

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1607: int R = (write == 0) ? B22 : 0;
since write is just a flag, should it be a bool rather than an int? or an enum?

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1612: emit(0xf510f000 | B16 * base.code() | U | R |
offset);
I think "base.code()*B16" would be more in line with the rest of the assembler.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2667: 
v8 likes to have two empty lines between each function definition; this applies
below too

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2730: int Vd = first.code() & 0xf;
I think it's nicer to use DwVfpRegister::split to get vd and d; see e.g. how
vadd does this.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2731: // we don't need arbitrary Rm so far
nit: comments should ideally start with a capital letter and end with a period

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2733: int size_enc = bin_log(size) - 3;
Since size_enc isn't going to be used until further down in the function,
perhaps it can be moved closer to its first use?

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2735: (0xf << 28) | B26 | B22*d | B21*load |
B16*base.code() |
Instead of "0xf << 28", 0xf*B28 would be more in line with the rest of the
assembler.

Also, instead of doing "B22*d" etc, I think "d*B22" is more in line with the
rest of the assembler.

These comments apply where "result" is modified further down in this function
too.

https://codereview.chromium.org/12920009/diff/17001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/12920009/diff/17001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:121: // Based on Bionic's memcpy.s
period at the end of comments, here and below

[Nike, 2013-04-03 15:04:06]

Hopefully covered most of Hans suggestions, PTAL.

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

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1605: void Assembler::pld(Register base, int offset,
int write) {
On 2013/04/02 12:34:29, hans wrote:
> Many of the newly added functions to the assembler have references to the
> appropriate section in the ARM ARM, and also a comment that lays out the
> instruction encoding that makes it much easier to verify that the code is
> correct. See e.g. vadd below.
> 
> All new assembler functions also need corresponding changes in the simulator
and
> disassembler + tests.

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1605: void Assembler::pld(Register base, int offset,
int write) {
Done description, is it OK to make simulator+test changes in separate CL?

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1606: int U = (offset >= 0) ? B23 : 0;
On 2013/04/02 12:34:29, hans wrote:
> looks like indentation is too deep in this function

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1607: int R = (write == 0) ? B22 : 0;
On 2013/04/02 12:34:29, hans wrote:
> since write is just a flag, should it be a bool rather than an int? or an
enum?

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1612: emit(0xf510f000 | B16 * base.code() | U | R |
offset);
On 2013/04/02 12:34:29, hans wrote:
> I think "base.code()*B16" would be more in line with the rest of the
assembler.

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2667: 
On 2013/04/02 12:34:29, hans wrote:
> v8 likes to have two empty lines between each function definition; this
applies
> below too

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2730: int Vd = first.code() & 0xf;
On 2013/04/02 12:34:29, hans wrote:
> I think it's nicer to use DwVfpRegister::split to get vd and d; see e.g. how
> vadd does this.

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2731: // we don't need arbitrary Rm so far
On 2013/04/02 12:34:29, hans wrote:
> nit: comments should ideally start with a capital letter and end with a period

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2733: int size_enc = bin_log(size) - 3;
On 2013/04/02 12:34:29, hans wrote:
> Since size_enc isn't going to be used until further down in the function,
> perhaps it can be moved closer to its first use?

Done.

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2735: (0xf << 28) | B26 | B22*d | B21*load |
B16*base.code() |
B22 done.
Using 0xf*B28 gives 
error: integer overflow in expression [-Werror=overflow]

On 2013/04/02 12:34:29, hans wrote:
> Instead of "0xf << 28", 0xf*B28 would be more in line with the rest of the
> assembler.
> 
> Also, instead of doing "B22*d" etc, I think "d*B22" is more in line with the
> rest of the assembler.
> 
> These comments apply where "result" is modified further down in this function
> too.

https://codereview.chromium.org/12920009/diff/17001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/12920009/diff/17001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:121: // Based on Bionic's memcpy.s
On 2013/04/02 12:34:29, hans wrote:
> period at the end of comments, here and below

Done.

[hans, 2013-04-04 09:56:51]

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

https://codereview.chromium.org/12920009/diff/17001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:1605: void Assembler::pld(Register base, int offset,
int write) {
On 2013/04/03 15:04:06, Nike wrote:
> Done description,

Thanks! This makes it much easier to verify the code.

> is it OK to make simulator+test changes in separate CL? 

No, the preferred way is to do it in the same patch. If V8 starts generating
code which it cannot simulate or disassemble, that would be a problem. If it's
not tested, that's also a problem.

[hans, 2013-04-05 09:33:15]

On 2013/04/04 09:56:51, hans wrote:
> > is it OK to make simulator+test changes in separate CL? 
> 
> No, the preferred way is to do it in the same patch. If V8 starts generating
> code which it cannot simulate or disassemble, that would be a problem. If it's
> not tested, that's also a problem.

There is another patch in flight that adds the PLD instruction:
https://chromiumcodereview.appspot.com/12367002/
Maybe this patch can build on that, once it has landed?

[Nike, 2013-04-12 14:50:12]

Sure, once I'll complete with tests and disassembler support (got really busy
with other projects), we could commit this CL in current form, and later switch
to another PLD, or if CL in question will be integrated - I can just use it.
BTW, thanks for the link to CL - it gives good insight on what to be done to
implement disassembler and emulator support.

[ulan, 2013-04-26 14:21:31]

Added Rodolph and myself to reviewers.

High level question: did you run the benchmarks (Octane, Kraken, SS) with this?

As mentioned by Hans, we need the disassembler support for the new instructions
and tests.

There were recent changes in ARM feature detection:
https://code.google.com/p/v8/source/detail?r=14288

It might make sense to rebase on top of that.

[Rodolph Perfetta, 2013-04-30 17:08:36]

comments for the neon code.

We have a similar patch in the work (stalled for now), let us know if you are
interested in merging our efforts.

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:124: static const int kCacheLineSize = 64;
This is true on A8 and A15 but not A9.

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:155: ASSERT(OS::kMinComplexMemCopy >= 16);
STATIC_ASSERT

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:173: __ ldrb(lr, MemOperand(r1, 1, PostIndex), cs);
use ldrh instead of 2 ldrb.

Same for stores.

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:179: __ vld4(8, r1, d0, element_0, Writeback);
I am not sure why you are using vld4. Currently you are loading a byte into
d0[0], the next byte into d1[0], etc. So you are touching 4 double registers to
load 4 bytes. If you want to load a word then use ldr. It also saves you
implementing vld4 in the assembler/disasm/simulator.

If you want to avoid mixing arm/neon then you should also get rid of he
ldrb/strb above.

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:226: __ b(&has32, hs);
If I followed correctly when you enter the has32 block, you know that 32 <= r2 <
64, so this branch will never be taken.

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:244: __ b(&skip_copy4, ge);
ge implies N flag == V flag, shift with SetCC don't touch the overflow flag so
you probably meant to use pl (N clear).

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:251: __ ldrb(lr, MemOperand(r1, 1, PostIndex), cs);
ldrh, then strh below.

https://codereview.chromium.org/12920009/diff/23001/src/arm/codegen-arm.cc#ne...
src/arm/codegen-arm.cc:257: __ bx(lr);
You can combine both operations above with:
  __ pop(pc);