ARM64: Add NEON support

ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

BUG=

Review-Url: https://codereview.chromium.org/2622643005
Cr-Commit-Position: refs/heads/master@{#44306}
Committed: https://chromium.googlesource.com/v8/v8/+/4506382dceb2cad7d2f5d53aca3b368c860f06cb

[martyn.capewell, 2017-01-10 10:44:15]

martyn.capewell@arm.com changed reviewers:
+ bbudge@chromium.org, bmeurer@chromium.org, epertoso@chromium.org

[bradn, 2017-01-20 01:00:28]

Description was changed from

==========
ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

This patch does not include the large set of trace files required by the tests.
As such, it will not pass tests as it stands.

BUG=
==========

to

==========
ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

This patch does not include the large set of trace files required by the tests.
As such, it will not pass tests as it stands.

BUG=
==========

[bradn, 2017-01-20 01:00:28]

bradnelson@google.com changed reviewers:
+ beckie@google.com

[bradn, 2017-01-20 01:00:36]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bbudge, 2017-01-31 01:41:32]

This is a first round of comments. In general, the patch looks good, and I
appreciate the amount of work that went into it, especially the extensive tests.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.c...
src/arm64/assembler-arm64.cc:280: return match;
Rather than use integer & operator, could you return an expression, i.e.

return (!reg2.IsValid() || reg2.IsSameFormat(reg1)) &&
       (!reg3.IsValid() || reg3.IsSameFormat(reg1)) ... etc

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.c...
src/arm64/assembler-arm64.cc:286: if (!reg2.IsValid()) {
DCHECK(!reg3.IsValid() && !reg4.IsValid()) ?

similarly below.

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

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:57: V(q8)  V(q9)  V(q10) V(q11) V(q12) V(q13) V(q14)
V(q15)
Can we remove this?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:505: // consecutive in the register file. Arguments
set to NoReg are ignored, as are
Current behavior is to return 'true' after the first NoReg.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:3028: return 0xffffffff;  // Undefined instruction.
could we define a kUndefinedInstruction?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:3271: NEONTableOp op);
Could this be private instead of protected?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc
File src/arm64/disasm-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:1383: {23, 22, 30}, {NF_4H, NF_8H, NF_2S, NF_4S,
NF_1D, NF_2D}};
Could you define some constants to make it clearer what these numbers mean?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:1819: static const NEONFormatMap map_ta = {{23, 22},
{NF_UNDEF, NF_4S, NF_2D}};
magic numbers

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:2941: NEONFormatMap map = {{22}, {NF_2S, NF_2D}};
magic numbers

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:2976: {22, 21, 20, 19},
magic numbers

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:3085: {22, 21, 20, 19},
magic numbers

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:3271: char re_form[sizeof(form_4v) + 6];
magic number (6)

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.h
File src/arm64/disasm-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.h#ne...
src/arm64/disasm-arm64.h:38: // TODO(mcapewel): This currently doesn't allow
renaming of V registers.
nit: mcapewell?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.h#ne...
src/arm64/disasm-arm64.h:39: virtual void AppendRegisterNameToOutput(const
Instruction* instr,
nit: const -> &

https://codereview.chromium.org/2622643005/diff/1/src/arm64/instructions-arm64.h
File src/arm64/instructions-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/instructions-arm6...
src/arm64/instructions-arm64.h:483: const int kMaxLanesPerVector = 16;
It seems kind of random to have this here. Could this be moved to
constants-arm64.h or logic-arm64.cc?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/instructions-arm6...
src/arm64/instructions-arm64.h:648: class NEONFormatDecoder {
Is there a reason so much of this class is defined in the header?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc
File src/arm64/logic-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:8: #include "src/arm64/simulator-arm64.h"
Did you pull this out of simulator-arm64.cc because of file size? Perhaps the
file name should reflect this and cause the files to be grouped together
alphabetically, e.g.
simulator-neon-arm64.cc
simulator-logic-arm64.cc

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:18: }
Why not define these inline in the header, simulator-arm64.h, since they're used
there?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:50: }  // anonymous namespace
nit: just 'namespace'

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:112: //  - The payload (mantissa) is transferred
entirely, except that the top
s/payload/mantissa ?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:322: //    that the top bit is forced to '1', making
the result a quiet NaN.
s/payload/mantissa

https://codereview.chromium.org/2622643005/diff/1/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/simulator-arm64.c...
src/arm64/simulator-arm64.cc:4539: if ((instr->Bit(23) == 0) && (instr->Bits(20,
16) != 0)) {
DCHECK_IMPLIES ?

https://codereview.chromium.org/2622643005/diff/1/src/crankshaft/arm64/lithiu...
File src/crankshaft/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/crankshaft/arm64/lithiu...
src/crankshaft/arm64/lithium-codegen-arm64.cc:1137: if
(masm()->IsImmLSScaled(jssp_offset, 3)) {
nit: replace magic number with named constant

https://codereview.chromium.org/2622643005/diff/1/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/cctest.h#newcod...
test/cctest/cctest.h:174: static bool sim_test_trace_;
As far as I can tell, this will always be false.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm64.cc:236: #define CHECK_EQUAL_128(expected_h,
expected_l, result) \
I dug around to figure out that _h and _l stand for high / low. Could you spell
that out here?

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
File test/cctest/test-simulator-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:52: // utils-arm64.cc for more info on
RegisterDump.
It seems to be in test-utils-arm64.h

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:56: //   CHECK_EQUAL_32(int32_t, int_32t)
nit int32_t

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:60: //   CHECK_EQUAL_64(int64_t, int_64t)
int64_t

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:329: Td* results = new Td[results_length *
vd_lane_count];
Use std::vector rather than manage raw array pointers.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:579: // This output for the 'Across'
category has the required modifications.
It's not clear what is "to be done" here.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
File test/cctest/test-simulator-inputs-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-inputs-arm64.h:14: // doesn't need the usual header
guard.
Why have a header at all then?

[martyn.capewell, 2017-02-03 11:01:32]

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.c...
src/arm64/assembler-arm64.cc:280: return match;
On 2017/01/31 01:41:31, bbudge wrote:
> Rather than use integer & operator, could you return an expression, i.e.
> 
> return (!reg2.IsValid() || reg2.IsSameFormat(reg1)) &&
>        (!reg3.IsValid() || reg3.IsSameFormat(reg1)) ... etc

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.c...
src/arm64/assembler-arm64.cc:286: if (!reg2.IsValid()) {
On 2017/01/31 01:41:31, bbudge wrote:
> DCHECK(!reg3.IsValid() && !reg4.IsValid()) ?
> 
> similarly below.

Done.

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

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:57: V(q8)  V(q9)  V(q10) V(q11) V(q12) V(q13) V(q14)
V(q15)
On 2017/01/31 01:41:31, bbudge wrote:
> Can we remove this?

This is used by register-configuration.cc. Why do you want to remove it?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:505: // consecutive in the register file. Arguments
set to NoReg are ignored, as are
On 2017/01/31 01:41:31, bbudge wrote:
> Current behavior is to return 'true' after the first NoReg.

Not sure what you're asking - are you saying the comment's wrong, it should be
clearer, or, after the new assertions in the implementation, it needs to be
corrected?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:3028: return 0xffffffff;  // Undefined instruction.
On 2017/01/31 01:41:31, bbudge wrote:
> could we define a kUndefinedInstruction?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:3271: NEONTableOp op);
On 2017/01/31 01:41:31, bbudge wrote:
> Could this be private instead of protected?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc
File src/arm64/disasm-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:1383: {23, 22, 30}, {NF_4H, NF_8H, NF_2S, NF_4S,
NF_1D, NF_2D}};
On 2017/01/31 01:41:32, bbudge wrote:
> Could you define some constants to make it clearer what these numbers mean?

They're just bit positions in the instruction, so don't have useful names. I
could reuse the B0-B31 enumeration that the ARM backend uses for instruction
encoding.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:3271: char re_form[sizeof(form_4v) + 6];
On 2017/01/31 01:41:32, bbudge wrote:
> magic number (6)

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.h
File src/arm64/disasm-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.h#ne...
src/arm64/disasm-arm64.h:38: // TODO(mcapewel): This currently doesn't allow
renaming of V registers.
On 2017/01/31 01:41:31, bbudge wrote:
> nit: mcapewell?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.h#ne...
src/arm64/disasm-arm64.h:39: virtual void AppendRegisterNameToOutput(const
Instruction* instr,
On 2017/01/31 01:41:31, bbudge wrote:
> nit: const -> &

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/instructions-arm64.h
File src/arm64/instructions-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/instructions-arm6...
src/arm64/instructions-arm64.h:483: const int kMaxLanesPerVector = 16;
On 2017/01/31 01:41:31, bbudge wrote:
> It seems kind of random to have this here. Could this be moved to
> constants-arm64.h or logic-arm64.cc?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/instructions-arm6...
src/arm64/instructions-arm64.h:648: class NEONFormatDecoder {
On 2017/01/31 01:41:31, bbudge wrote:
> Is there a reason so much of this class is defined in the header?

I've moved all but the default format maps out of the header.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc
File src/arm64/logic-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:8: #include "src/arm64/simulator-arm64.h"
On 2017/01/31 01:41:32, bbudge wrote:
> Did you pull this out of simulator-arm64.cc because of file size? Perhaps the
> file name should reflect this and cause the files to be grouped together
> alphabetically, e.g.
> simulator-neon-arm64.cc
> simulator-logic-arm64.cc

This file contains the more complicated simulator operations, some of which were
already defined for floating point instructions, but needed to be iterated over
lanes for NEON.

I think these were originally in a separate class, but they needed to share too
much information with the Simulator, so they returned to the Simulator, but
stayed in this new file, as there was still a conceptual gap. I can rename this
file simulator-logic-arm64.cc, if that works?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:18: }
On 2017/01/31 01:41:32, bbudge wrote:
> Why not define these inline in the header, simulator-arm64.h, since they're
used
> there?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:50: }  // anonymous namespace
On 2017/01/31 01:41:32, bbudge wrote:
> nit: just 'namespace'

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:112: //  - The payload (mantissa) is transferred
entirely, except that the top
On 2017/01/31 01:41:32, bbudge wrote:
> s/payload/mantissa ?

Not sure what you mean here. In the context of a NaN, it's called a "payload",
as it has no numerical meaning. It just happens to occupy the same bits as a
mantissa would were the representation a number.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/simulator-arm64.cc
File src/arm64/simulator-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/simulator-arm64.c...
src/arm64/simulator-arm64.cc:4539: if ((instr->Bit(23) == 0) && (instr->Bits(20,
16) != 0)) {
On 2017/01/31 01:41:31, bbudge wrote:
> DCHECK_IMPLIES ?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/crankshaft/arm64/lithiu...
File src/crankshaft/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/crankshaft/arm64/lithiu...
src/crankshaft/arm64/lithium-codegen-arm64.cc:1137: if
(masm()->IsImmLSScaled(jssp_offset, 3)) {
On 2017/01/31 01:41:31, bbudge wrote:
> nit: replace magic number with named constant

Done.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/cctest.h#newcod...
test/cctest/cctest.h:174: static bool sim_test_trace_;
On 2017/01/31 01:41:31, bbudge wrote:
> As far as I can tell, this will always be false.

This is a leftover from generating the reference trace outputs. I'll remove it
all, as I don't expect V8 to need to regenerate.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm64.cc:236: #define CHECK_EQUAL_128(expected_h,
expected_l, result) \
On 2017/01/31 01:41:31, bbudge wrote:
> I dug around to figure out that _h and _l stand for high / low. Could you
spell
> that out here?

Done.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
File test/cctest/test-simulator-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:52: // utils-arm64.cc for more info on
RegisterDump.
On 2017/01/31 01:41:32, bbudge wrote:
> It seems to be in test-utils-arm64.h

Done.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:56: //   CHECK_EQUAL_32(int32_t, int_32t)
On 2017/01/31 01:41:32, bbudge wrote:
> nit int32_t

Done.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:60: //   CHECK_EQUAL_64(int64_t, int_64t)
On 2017/01/31 01:41:32, bbudge wrote:
> int64_t

Done.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:329: Td* results = new Td[results_length *
vd_lane_count];
On 2017/01/31 01:41:31, bbudge wrote:
> Use std::vector rather than manage raw array pointers.

I could do this, but I'd need to push dummy values into the vector before
passing it into the helper, as the helper fills in the results from jitted code.
The only way I can see to do that is a loop on push_back(), which seems messy.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:579: // This output for the 'Across'
category has the required modifications.
On 2017/01/31 01:41:31, bbudge wrote:
> It's not clear what is "to be done" here.

I think it's saying that, on error, it prints out a whole vector, because it
doesn't take account of the input format. If it did look at the format, only the
elements we care about would be printed. However, I'll remove the TODO, as it's
just confusing, and covers a minor improvement to error reporting.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
File test/cctest/test-simulator-inputs-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-inputs-arm64.h:14: // doesn't need the usual header
guard.
On 2017/01/31 01:41:31, bbudge wrote:
> Why have a header at all then?

Are you suggesting pasting this file into the top of test-simulator-arm64.cc? I
can do that, though it's already a long file, and keeping the input values in
here might be tidier. You choose.

[bbudge, 2017-02-08 01:39:12]

Just a few comments. Hopefully I've answered your questions. Looking good.

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

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:57: V(q8)  V(q9)  V(q10) V(q11) V(q12) V(q13) V(q14)
V(q15)
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:31, bbudge wrote:
> > Can we remove this?
> 
> This is used by register-configuration.cc. Why do you want to remove it?

Ah, you're right. Don't we have 32 V registers though?

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:505: // consecutive in the register file. Arguments
set to NoReg are ignored, as are
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:31, bbudge wrote:
> > Current behavior is to return 'true' after the first NoReg.
> 
> Not sure what you're asking - are you saying the comment's wrong, it should be
> clearer, or, after the new assertions in the implementation, it needs to be
> corrected?

Comment is OK.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc
File src/arm64/disasm-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:1383: {23, 22, 30}, {NF_4H, NF_8H, NF_2S, NF_4S,
NF_1D, NF_2D}};
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:32, bbudge wrote:
> > Could you define some constants to make it clearer what these numbers mean?
> 
> They're just bit positions in the instruction, so don't have useful names. I
> could reuse the B0-B31 enumeration that the ARM backend uses for instruction
> encoding.

I see. It's OK like this then.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:1819: static const NEONFormatMap map_ta = {{23, 22},
{NF_UNDEF, NF_4S, NF_2D}};
On 2017/01/31 01:41:32, bbudge wrote:
> magic numbers

OK

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:2941: NEONFormatMap map = {{22}, {NF_2S, NF_2D}};
On 2017/01/31 01:41:32, bbudge wrote:
> magic numbers

OK

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:2976: {22, 21, 20, 19},
On 2017/01/31 01:41:32, bbudge wrote:
> magic numbers

OK

https://codereview.chromium.org/2622643005/diff/1/src/arm64/disasm-arm64.cc#n...
src/arm64/disasm-arm64.cc:3085: {22, 21, 20, 19},
On 2017/01/31 01:41:32, bbudge wrote:
> magic numbers

OK

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc
File src/arm64/logic-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:8: #include "src/arm64/simulator-arm64.h"
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:32, bbudge wrote:
> > Did you pull this out of simulator-arm64.cc because of file size? Perhaps
the
> > file name should reflect this and cause the files to be grouped together
> > alphabetically, e.g.
> > simulator-neon-arm64.cc
> > simulator-logic-arm64.cc
> 
> This file contains the more complicated simulator operations, some of which
were
> already defined for floating point instructions, but needed to be iterated
over
> lanes for NEON.
> 
> I think these were originally in a separate class, but they needed to share
too
> much information with the Simulator, so they returned to the Simulator, but
> stayed in this new file, as there was still a conceptual gap. I can rename
this
> file simulator-logic-arm64.cc, if that works?

That's fine.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:112: //  - The payload (mantissa) is transferred
entirely, except that the top
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:32, bbudge wrote:
> > s/payload/mantissa ?
> 
> Not sure what you mean here. In the context of a NaN, it's called a "payload",
> as it has no numerical meaning. It just happens to occupy the same bits as a
> mantissa would were the representation a number.

For consistency. It looks like 'mantissa' is used in the other conversion
functions here.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
File test/cctest/test-simulator-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:329: Td* results = new Td[results_length *
vd_lane_count];
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:31, bbudge wrote:
> > Use std::vector rather than manage raw array pointers.
> 
> I could do this, but I'd need to push dummy values into the vector before
> passing it into the helper, as the helper fills in the results from jitted
code.
> The only way I can see to do that is a loop on push_back(), which seems messy.

If the types Td are default constructible, you can use the fill constructor for
std::vector.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:579: // This output for the 'Across'
category has the required modifications.
On 2017/02/03 11:01:31, martyn.capewell wrote:
> On 2017/01/31 01:41:31, bbudge wrote:
> > It's not clear what is "to be done" here.
> 
> I think it's saying that, on error, it prints out a whole vector, because it
> doesn't take account of the input format. If it did look at the format, only
the
> elements we care about would be printed. However, I'll remove the TODO, as
it's
> just confusing, and covers a minor improvement to error reporting.

OK

[martyn.capewell, 2017-02-15 11:51:05]

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

https://codereview.chromium.org/2622643005/diff/1/src/arm64/assembler-arm64.h...
src/arm64/assembler-arm64.h:57: V(q8)  V(q9)  V(q10) V(q11) V(q12) V(q13) V(q14)
V(q15)
On 2017/02/08 01:39:11, bbudge wrote:
> On 2017/02/03 11:01:31, martyn.capewell wrote:
> > On 2017/01/31 01:41:31, bbudge wrote:
> > > Can we remove this?
> > 
> > This is used by register-configuration.cc. Why do you want to remove it?
> 
> Ah, you're right. Don't we have 32 V registers though?

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc
File src/arm64/logic-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:8: #include "src/arm64/simulator-arm64.h"
On 2017/02/08 01:39:11, bbudge wrote:
> On 2017/02/03 11:01:31, martyn.capewell wrote:
> > On 2017/01/31 01:41:32, bbudge wrote:
> > > Did you pull this out of simulator-arm64.cc because of file size? Perhaps
> the
> > > file name should reflect this and cause the files to be grouped together
> > > alphabetically, e.g.
> > > simulator-neon-arm64.cc
> > > simulator-logic-arm64.cc
> > 
> > This file contains the more complicated simulator operations, some of which
> were
> > already defined for floating point instructions, but needed to be iterated
> over
> > lanes for NEON.
> > 
> > I think these were originally in a separate class, but they needed to share
> too
> > much information with the Simulator, so they returned to the Simulator, but
> > stayed in this new file, as there was still a conceptual gap. I can rename
> this
> > file simulator-logic-arm64.cc, if that works?
> 
> That's fine.

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:112: //  - The payload (mantissa) is transferred
entirely, except that the top
On 2017/02/08 01:39:11, bbudge wrote:
> On 2017/02/03 11:01:31, martyn.capewell wrote:
> > On 2017/01/31 01:41:32, bbudge wrote:
> > > s/payload/mantissa ?
> > 
> > Not sure what you mean here. In the context of a NaN, it's called a
"payload",
> > as it has no numerical meaning. It just happens to occupy the same bits as a
> > mantissa would were the representation a number.
> 
> For consistency. It looks like 'mantissa' is used in the other conversion
> functions here.

Done.

https://codereview.chromium.org/2622643005/diff/1/src/arm64/logic-arm64.cc#ne...
src/arm64/logic-arm64.cc:322: //    that the top bit is forced to '1', making
the result a quiet NaN.
On 2017/01/31 01:41:32, bbudge wrote:
> s/payload/mantissa

Done.

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
File test/cctest/test-simulator-arm64.cc (right):

https://codereview.chromium.org/2622643005/diff/1/test/cctest/test-simulator-...
test/cctest/test-simulator-arm64.cc:329: Td* results = new Td[results_length *
vd_lane_count];
On 2017/02/08 01:39:11, bbudge wrote:
> On 2017/02/03 11:01:31, martyn.capewell wrote:
> > On 2017/01/31 01:41:31, bbudge wrote:
> > > Use std::vector rather than manage raw array pointers.
> > 
> > I could do this, but I'd need to push dummy values into the vector before
> > passing it into the helper, as the helper fills in the results from jitted
> code.
> > The only way I can see to do that is a loop on push_back(), which seems
messy.
> 
> If the types Td are default constructible, you can use the fill constructor
for
> std::vector.

Done.

[bbudge, 2017-02-22 01:50:59]

Obviously I haven't thoroughly reviewed every line of this CL, but except for a
few comments, LGTM.

We should figure out how to add the traces and how you can land this.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/assembler-arm...
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/assembler-arm...
src/arm64/assembler-arm64.h:1319: // Bitwise orn.
nit: comments should contain more information, e.g. Bitwise not or.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/assembler-arm...
src/arm64/assembler-arm64.h:1322: // Bitwise eor.
nit exclusive or.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/constants-arm...
File src/arm64/constants-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/constants-arm...
src/arm64/constants-arm64.h:868: 
You could use // clang-format off and 'on' to preserve good formatting here.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.h:503: DCHECK(allow_macro_instructions_);
From the comments on this member variable, this should only be checked if
multiple instructions are generated. It appears that isn't the case here. (I
realize this pattern is repeated in all the existing code, just wondering what
is intended here.)

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:413: };
nit: whitespace

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:423: for (unsigned i = 0; i < sizeof(saturated_) /
sizeof(saturated_[0]); i++) {
Can you use kQRegSize instead of generating the length? Or memset?

https://codereview.chromium.org/2622643005/diff/20001/test/cctest/cctest.cc
File test/cctest/cctest.cc (right):

https://codereview.chromium.org/2622643005/diff/20001/test/cctest/cctest.cc#n...
test/cctest/cctest.cc:50: 
nit: remove whitespace

[martyn.capewell, 2017-02-23 13:10:41]

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/assembler-arm...
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/assembler-arm...
src/arm64/assembler-arm64.h:1319: // Bitwise orn.
On 2017/02/22 01:50:59, bbudge wrote:
> nit: comments should contain more information, e.g. Bitwise not or.

Done.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/assembler-arm...
src/arm64/assembler-arm64.h:1322: // Bitwise eor.
On 2017/02/22 01:50:59, bbudge wrote:
> nit exclusive or.

Done.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/constants-arm...
File src/arm64/constants-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/constants-arm...
src/arm64/constants-arm64.h:868: 
On 2017/02/22 01:50:59, bbudge wrote:
> You could use // clang-format off and 'on' to preserve good formatting here.

Done.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.h:503: DCHECK(allow_macro_instructions_);
The aim is to disallow using the macro assembler in InstructionAccurateScope,
which is meant to guarantee that there's a one to one mapping from instruction
requested to instruction generated.

Whilst most masm functions only generate one instruction, we thought it safer to
maintain the rule that the macro assembler may generate multiple instructions
per call, but the assembler only one per call.

The separation is then clear, and we can improve the macro assembler without
worrying about affecting the most difficult code to debug: instructions that are
changed after generation.

So, if you're writing code that is later patched, use InstructionAccurateScope
and the assembler. Otherwise, use the macro assembler.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/simulator-arm...
File src/arm64/simulator-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:413: };
On 2017/02/22 01:50:58, bbudge wrote:
> nit: whitespace

Done.

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/simulator-arm...
src/arm64/simulator-arm64.h:423: for (unsigned i = 0; i < sizeof(saturated_) /
sizeof(saturated_[0]); i++) {
On 2017/02/22 01:50:58, bbudge wrote:
> Can you use kQRegSize instead of generating the length? Or memset?

Done.

https://codereview.chromium.org/2622643005/diff/20001/test/cctest/cctest.cc
File test/cctest/cctest.cc (right):

https://codereview.chromium.org/2622643005/diff/20001/test/cctest/cctest.cc#n...
test/cctest/cctest.cc:50: 
On 2017/02/22 01:50:59, bbudge wrote:
> nit: remove whitespace

Done.

[bbudge, 2017-02-24 22:34:24]

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/macro-assembl...
File src/arm64/macro-assembler-arm64.h (right):

https://codereview.chromium.org/2622643005/diff/20001/src/arm64/macro-assembl...
src/arm64/macro-assembler-arm64.h:503: DCHECK(allow_macro_instructions_);
On 2017/02/23 13:10:41, martyn.capewell wrote:
> The aim is to disallow using the macro assembler in InstructionAccurateScope,
> which is meant to guarantee that there's a one to one mapping from instruction
> requested to instruction generated.
> 
> Whilst most masm functions only generate one instruction, we thought it safer
to
> maintain the rule that the macro assembler may generate multiple instructions
> per call, but the assembler only one per call.
> 
> The separation is then clear, and we can improve the macro assembler without
> worrying about affecting the most difficult code to debug: instructions that
are
> changed after generation.
> 
> So, if you're writing code that is later patched, use InstructionAccurateScope
> and the assembler. Otherwise, use the macro assembler.

OK

[martyn.capewell, 2017-03-16 11:25:57]

I've addressed the last of the comments, and added a DEPS entry for the traces
held in a new external repository. Other repositories are mirrored at
chromium.googlesource.com, so I don't know if this needs to happen too.

Also, I've still to rebase on current master.

[martyn.capewell, 2017-03-16 14:37:55]

Description was changed from

==========
ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

This patch does not include the large set of trace files required by the tests.
As such, it will not pass tests as it stands.

BUG=
==========

to

==========
ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

BUG=
==========

[bbudge, 2017-03-27 17:40:52]

Still LGTM.

[martyn.capewell, 2017-03-27 17:47:46]

The CQ bit was checked by martyn.capewell@arm.com

[commit-bot: I haz the power, 2017-03-27 17:47:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 17:49:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 17:49:27]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19846)

[martyn.capewell, 2017-03-28 13:11:34]

The CQ bit was unchecked by martyn.capewell@arm.com

[martyn.capewell, 2017-03-28 13:12:01]

The CQ bit was checked by martyn.capewell@arm.com

[martyn.capewell, 2017-03-28 13:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2622643005/#ps50001
(title: "Fix rebase conflicts")

[commit-bot: I haz the power, 2017-03-28 13:12:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 13:15:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 13:15:20]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/37848)

[martyn.capewell, 2017-03-30 12:33:37]

Hi Benedikt - can I have a lgtm on this to pass presubmit?

[Benedikt Meurer, 2017-03-30 12:37:00]

LGTM (rubber-stamped)

[martyn.capewell, 2017-03-30 12:38:58]

The CQ bit was checked by martyn.capewell@arm.com

[martyn.capewell, 2017-03-30 12:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2622643005/#ps70001
(title: "Fix mozilla test and presubmit")

[commit-bot: I haz the power, 2017-03-30 12:39:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 13:10:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 13:11:00]

Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)

[martyn.capewell, 2017-03-31 10:10:20]

The CQ bit was unchecked by martyn.capewell@arm.com

[martyn.capewell, 2017-03-31 10:10:58]

The CQ bit was checked by martyn.capewell@arm.com

[martyn.capewell, 2017-03-31 10:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2622643005/#ps80001
(title: "Fix Math.abs properly")

[commit-bot: I haz the power, 2017-03-31 10:11:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 11:33:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490955058881180,
"parent_rev": "04f48a76c87c464a88f049fe64fb2251c6108fb6", "commit_rev":
"4506382dceb2cad7d2f5d53aca3b368c860f06cb"}

[commit-bot: I haz the power, 2017-03-31 11:33:12]

Description was changed from

==========
ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

BUG=
==========

to

==========
ARM64: Add NEON support

Add assembler, disassembler and simulator support for NEON in the ARM64 backend.

BUG=

Review-Url: https://codereview.chromium.org/2622643005
Cr-Commit-Position: refs/heads/master@{#44306}
Committed:
https://chromium.googlesource.com/v8/v8/+/4506382dceb2cad7d2f5d53aca3b368c860...
==========

[commit-bot: I haz the power, 2017-03-31 11:33:14]

Committed patchset #9 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/4506382dceb2cad7d2f5d53aca3b368c860...

[Michael Achenbach, 2017-03-31 12:36:37]

On 2017/03/31 11:33:14, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:80001) as
>
https://chromium.googlesource.com/v8/v8/+/4506382dceb2cad7d2f5d53aca3b368c860...

Revert CL:
https://codereview.chromium.org/2785183005

For the deps, please file a ticket to chromium issue tracker with component
Infra>Git for requesting a googlesource mirror.

[martyn.capewell, 2017-03-31 13:05:41]

On 2017/03/31 12:36:37, Michael Achenbach wrote:
> On 2017/03/31 11:33:14, commit-bot: I haz the power wrote:
> > Committed patchset #9 (id:80001) as
> >
>
https://chromium.googlesource.com/v8/v8/+/4506382dceb2cad7d2f5d53aca3b368c860...
> 
> Revert CL:
> https://codereview.chromium.org/2785183005
> 
> For the deps, please file a ticket to chromium issue tracker with component
> Infra>Git for requesting a googlesource mirror.

OK. Created https://bugs.chromium.org/p/v8/issues/detail?id=6185 though I can't
see how to set the component from the interface I have.