Begins work on ARM64, first assembler test.

Begins work on ARM64, first assembler test.

Most new code is in constants_arm64.h and
{assembler,disassembler,simulator}_arm64.{h,cc}

The rest of the CL just #def's out tests, modifies status files,
and adds UNIMPLEMENTED functions, etc.

R=regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=34654

[zra, 2014-04-01 18:04:13]

Lots of files touched, but the core of the change is not too big.

[regis, 2014-04-01 19:52:52]

Preliminary comments. The numerous file changes look good.
Give me some time to read more about the architecture before I review the
assembler,disassembler, and simulator.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/cpu_arm64.cc
File runtime/vm/cpu_arm64.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/cpu_arm64.cc#...
runtime/vm/cpu_arm64.cc:32: "sim"
Should this be "simarm64", as expected in cpu_test.cc? Why is it not failing,
btw?

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart.cc#newco...
runtime/vm/dart.cc:236: #if !defined(TARGET_ARCH_ARM64)
You are adding 2 types of such ifdef's.
The permanent ones and the temporary ones (such as this one) until arm64 is
fully implemented. I would mark the temporary ones with a TODO(zra), especially
when they are not at the top of a file.
It will be easier for you later to hunt for them again.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart_entry.cc...
runtime/vm/dart_entry.cc:68: #else
I'm tempted to say let's change these types to intptr_t, since we are only
supporting a 32-bit simulator on a 32-bit host, and a 64-bit simulator on a
64-bit host, but actually, clarity and verbosity will be your friends while
debugging. So I am not saying anything.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/stub_code.cc
File runtime/vm/stub_code.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/stub_code.cc#...
runtime/vm/stub_code.cc:54: #if !defined(TARGET_ARCH_ARM64)
Add TODO(zra)

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/stub_code.cc#...
runtime/vm/stub_code.cc:72: #if !defined(TARGET_ARCH_ARM64)
ditto

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.cc#...
runtime/vm/unit_test.cc:52: #if !defined(TARGET_ARCH_ARM64)
TODO(zra): Remove once supported.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.cc#...
runtime/vm/unit_test.cc:172: #if !defined(TARGET_ARCH_ARM64)
ditto

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.h#n...
runtime/vm/unit_test.h:162: #define EXECUTE_TEST_CODE_INT64_LL(name, entry,
long_arg0, long_arg1)          \
I suppose you will provide 64-bit versions of these macros later as they are
needed? Add a TODO to indicate so.

[zra, 2014-04-01 20:35:07]

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/cpu_arm64.cc
File runtime/vm/cpu_arm64.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/cpu_arm64.cc#...
runtime/vm/cpu_arm64.cc:32: "sim"
On 2014/04/01 19:52:53, regis wrote:
> Should this be "simarm64", as expected in cpu_test.cc? Why is it not failing,
> btw?

The "arm64" below is included unconditionally, making the full string "simarm64"
when HOST_ARCH_ARM64 is undefined. (This is just copied from cpu_arm.cc with 64
tacked on the end.)

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart.cc#newco...
runtime/vm/dart.cc:236: #if !defined(TARGET_ARCH_ARM64)
On 2014/04/01 19:52:53, regis wrote:
> You are adding 2 types of such ifdef's.
> The permanent ones and the temporary ones (such as this one) until arm64 is
> fully implemented. I would mark the temporary ones with a TODO(zra),
especially
> when they are not at the top of a file.
> It will be easier for you later to hunt for them again.

Done.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/dart_entry.cc...
runtime/vm/dart_entry.cc:68: #else
On 2014/04/01 19:52:53, regis wrote:
> I'm tempted to say let's change these types to intptr_t, since we are only
> supporting a 32-bit simulator on a 32-bit host, and a 64-bit simulator on a
> 64-bit host, but actually, clarity and verbosity will be your friends while
> debugging. So I am not saying anything.

I'll add a TODO.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/stub_code.cc
File runtime/vm/stub_code.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/stub_code.cc#...
runtime/vm/stub_code.cc:54: #if !defined(TARGET_ARCH_ARM64)
On 2014/04/01 19:52:53, regis wrote:
> Add TODO(zra)

Done.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/stub_code.cc#...
runtime/vm/stub_code.cc:72: #if !defined(TARGET_ARCH_ARM64)
On 2014/04/01 19:52:53, regis wrote:
> ditto

Done.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.cc#...
runtime/vm/unit_test.cc:52: #if !defined(TARGET_ARCH_ARM64)
On 2014/04/01 19:52:53, regis wrote:
> TODO(zra): Remove once supported.

Done.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.cc#...
runtime/vm/unit_test.cc:172: #if !defined(TARGET_ARCH_ARM64)
On 2014/04/01 19:52:53, regis wrote:
> ditto

Done.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/unit_test.h#n...
runtime/vm/unit_test.h:162: #define EXECUTE_TEST_CODE_INT64_LL(name, entry,
long_arg0, long_arg1)          \
On 2014/04/01 19:52:53, regis wrote:
> I suppose you will provide 64-bit versions of these macros later as they are
> needed? Add a TODO to indicate so.

Done.

[regis, 2014-04-01 22:25:14]

Looks good, but let me know what you think about my suggestions. We can discuss
in person tomorrow.

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/cpu_arm64.cc
File runtime/vm/cpu_arm64.cc (right):

https://codereview.chromium.org/221133002/diff/40001/runtime/vm/cpu_arm64.cc#...
runtime/vm/cpu_arm64.cc:32: "sim"
On 2014/04/01 20:35:08, zra wrote:
> On 2014/04/01 19:52:53, regis wrote:
> > Should this be "simarm64", as expected in cpu_test.cc? Why is it not
failing,
> > btw?
> 
> The "arm64" below is included unconditionally, making the full string
"simarm64"
> when HOST_ARCH_ARM64 is undefined. (This is just copied from cpu_arm.cc with
64
> tacked on the end.)

I see. Tricky. An #else would not kill us.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:109: class AddSubOperand : public ValueObject {
Is AddSubOperand a good name?
There are more instructions than just Add and Sub using a shifted register,
extended register, or immediate operand.

Why not Operand for now?
Do you expect more operand flavors?

On the arm side, we only have a ShifterOperand, that we could have called
Operand.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:152: enum ASOpType {
Does this mean AddSubOperandType?

How about OperandType or OperandKind?

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:254: void addi(OperandSize os, Register rd,
Register rn, AddSubOperand aso) {
Passing OperandSize explicitly seems strange to me.
OperandSize has ~9 different values, but add has only 2:
add x0, ... (using the official notation for 64-bit operand size) 
add w0, ... (using the official notation for 32-bit operand size)

For load instructions on arm, we did not specify the operand size explicitly,
but we have ldr, ldrb, ldrh, ldrsb, etc... 

I suspect that we will mostly emit 64-bit operand instructions, and not too many
32-bit operand instructions. So we could have:

add R0, ... (64-bit)
addw R0, ... (32-bit)

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:255: ASSERT(aso.type() ==
AddSubOperand::ASImmediate);
We will probably have a "macro" instruction AddImmediate taking an int32_t
directly (or rather intptr_t), but you should not introduce an addi at this
level.

I'd rather see:
void add(Register rd, Register rn, Operand x)
void addw(Register rd, Register rn, Operand x)

and
AddImmediate((Register rd, Register rn, intptr_t imm)
AddWImmediate((Register rd, Register rn, intptr_t imm)

with the macros emitting an add or addw with an Operand(imm), and possibly
emitting several instructions for arbitrary immediate values not fitting in the
12-bit field + shift.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/disassembler_...
File runtime/vm/disassembler_arm64.cc (right):

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/disassembler_...
runtime/vm/disassembler_arm64.cc:221: Print("x");
You could skip printing the 'x', to be in sync with the assembler.
On the other hand, we do not need to be in sync and verbosity is not bad when
disassembling.

[zra, 2014-04-02 00:04:25]

The naming suggestions make sense.

I noticed that I need to be a lot more careful with ZR/SP/R31 since there are
subtle differences in add with extend/shift/immediate. To enforce, I've made ZR
and SP abstract, and forbade R31 as an argument to add(). In add() I ASSERT that
ZR or SP is okay, as appropriate, and convert to R31 before Emitting. Hopefully
this will avoid mistakes when using add() and sub().

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:109: class AddSubOperand : public ValueObject {
On 2014/04/01 22:25:15, regis wrote:
> Is AddSubOperand a good name?
> There are more instructions than just Add and Sub using a shifted register,
> extended register, or immediate operand.
> 
> Why not Operand for now?
> Do you expect more operand flavors?
> 
> On the arm side, we only have a ShifterOperand, that we could have called
> Operand.

I had anticipated the bitfield immediate operand type to be sufficiently
different that a separate operand class might be useful, but on second look, I
think it's not really so different. I've renamed this to Operand.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:152: enum ASOpType {
On 2014/04/01 22:25:15, regis wrote:
> Does this mean AddSubOperandType?
> 
> How about OperandType or OperandKind?

Done.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:254: void addi(OperandSize os, Register rd,
Register rn, AddSubOperand aso) {
On 2014/04/01 22:25:15, regis wrote:
> Passing OperandSize explicitly seems strange to me.
> OperandSize has ~9 different values, but add has only 2:
> add x0, ... (using the official notation for 64-bit operand size) 
> add w0, ... (using the official notation for 32-bit operand size)
> 
> For load instructions on arm, we did not specify the operand size explicitly,
> but we have ldr, ldrb, ldrh, ldrsb, etc... 
> 
> I suspect that we will mostly emit 64-bit operand instructions, and not too
many
> 32-bit operand instructions. So we could have:
> 
> add R0, ... (64-bit)
> addw R0, ... (32-bit)

Since add() become more complicated, I added a helper function that accepts an
OperandSize to avoid duplicating code.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.h:255: ASSERT(aso.type() ==
AddSubOperand::ASImmediate);
On 2014/04/01 22:25:15, regis wrote:
> We will probably have a "macro" instruction AddImmediate taking an int32_t
> directly (or rather intptr_t), but you should not introduce an addi at this
> level.
> 
> I'd rather see:
> void add(Register rd, Register rn, Operand x)
> void addw(Register rd, Register rn, Operand x)
> 
> and
> AddImmediate((Register rd, Register rn, intptr_t imm)
> AddWImmediate((Register rd, Register rn, intptr_t imm)
> 
> with the macros emitting an add or addw with an Operand(imm), and possibly
> emitting several instructions for arbitrary immediate values not fitting in
the
> 12-bit field + shift. 

Done.

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/disassembler_...
File runtime/vm/disassembler_arm64.cc (right):

https://codereview.chromium.org/221133002/diff/60001/runtime/vm/disassembler_...
runtime/vm/disassembler_arm64.cc:221: Print("x");
On 2014/04/01 22:25:15, regis wrote:
> You could skip printing the 'x', to be in sync with the assembler.
> On the other hand, we do not need to be in sync and verbosity is not bad when
> disassembling.

I'll keep the x for now while I'm getting started, if it turns out we don't use
w very much, I'll remove. Added TODO.

[regis, 2014-04-02 00:14:49]

LGTM
Very good start!

[zra, 2014-04-02 17:36:08]

Thanks!

[zra, 2014-04-02 17:40:10]

Committed patchset #7 manually as r34654 (presubmit successful).