Use UnboxedInt32 and UnboxedUint32 representation for LoadIndexedInstr

- Use UnboxedInt32 and UnboxedUint32 representation for LoadIndexedInstr
- Avoid inserting an check class instruction for Uint32 and Int32 arrays on StoreIndexedInstr
- Implement Int32ToDouble on all architectures
- Add Int32 to double instruction to ARM64 and simulator.
- Add Int32 to double instruction to X64.

R=vegorov@google.com, zra@google.com

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

[Cutch, 2014-10-02 15:37:02]

johnmccutchan@google.com changed reviewers:
+ vegorov@google.com

[Cutch, 2014-10-02 15:57:02]

this is a resubmit of https://codereview.chromium.org/612553002/

PTAL- I made a change to the inlining of store indexed instruction to avoid an
unnecessary Box; CheckClass instruction sequence.

[Vyacheslav Egorov (Google), 2014-10-02 17:14:05]

https://codereview.chromium.org/616873003/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/616873003/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:87: return false;
How hard it is to support Int32ToDouble() on MIPS and ARM64? I suspect it's a
very simple instruction sequence (already available in both back ends as part of
UnboxDoubleInstr).

Please consider supporting it instead of including this check.

https://codereview.chromium.org/616873003/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:1392: if (!is_already_unboxed &&
!value_check.IsNull()) {
I am somewhat concerned about fragility of this.

Have you tried just making it: 

if ((array_cid != kTypedDataInt32ArrayCid) && (array_cid !=
kTypedDataUint32ArrayCid) ... ) here

1. UnboxInt32Instr should have class checks built in

2. There is a canonicalization rule doing Unbox(Box(v)) -> v

[Cutch, 2014-10-02 22:41:12]

PTAL

https://codereview.chromium.org/616873003/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/616873003/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:87: return false;
On 2014/10/02 17:14:04, Vyacheslav Egorov (Google) wrote:
> How hard it is to support Int32ToDouble() on MIPS and ARM64? I suspect it's a
> very simple instruction sequence (already available in both back ends as part
of
> UnboxDoubleInstr).
> 
> Please consider supporting it instead of including this check.

Done.

https://codereview.chromium.org/616873003/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:1392: if (!is_already_unboxed &&
!value_check.IsNull()) {
On 2014/10/02 17:14:04, Vyacheslav Egorov (Google) wrote:
> I am somewhat concerned about fragility of this.
> 
> Have you tried just making it: 
> 
> if ((array_cid != kTypedDataInt32ArrayCid) && (array_cid !=
> kTypedDataUint32ArrayCid) ... ) here
> 
> 1. UnboxInt32Instr should have class checks built in
> 
> 2. There is a canonicalization rule doing Unbox(Box(v)) -> v 

Done.

[Vyacheslav Egorov (Google), 2014-10-03 13:07:41]

lgtm

https://codereview.chromium.org/616873003/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm64.cc:4418: __ scvtfd(result, value);
Consider using 32-bit version of scvtfd (instruction is the same except 31-bit -
0 for 32-bit version and 1 for 64-bit version). See C3.6.30 in ARMv8 ISA

The reason for using 32-bit version is because truncating conversions right now
leave garbage in the upper bits under certain conditions.

https://codereview.chromium.org/616873003/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/616873003/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:4645: result->set_in(0,
Location::WritableRegister());
no need to ask for a writable register.

https://codereview.chromium.org/616873003/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:4654: __ cvtsi2sd(result, value);
The same comment as in ARM64. I suggest 32-bit version of cvtsi2sd to avoid
accidentally converting garbage in the upper word that can sometimes be there.

Encoding of 32-bit version is the same as 64-bit version but without a REX.W
prefix. (Volume 2A 3-211)

[Cutch, 2014-10-09 17:24:34]

johnmccutchan@google.com changed reviewers:
+ zra@google.com

[Cutch, 2014-10-09 17:24:44]

+zra for ARM64 changes.

[zra, 2014-10-09 18:00:31]

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:722: void scvtfd(VRegister vd, Register rn) {
scvtfdx to be consistent with x/w in other instructions.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:729: void scvtfd32(VRegister vd, Register rn) {
scvtfdw to be consistent with x/w in other instructions.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:733: EmitFPIntCvtOp(SCVTFD32,
static_cast<Register>(vd), crn);
Pass the size to EmitFPIntCvtOp instead of making a new opcode. Passing
kDoubleWord sets B31, passing kWord doesn't. Remove B31 from the opcodes in
FPIntCvtOp. This will make adding the 32-bit forms of the other instructions
easier if it turns out we need them, as well.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/disassembler...
File runtime/vm/disassembler_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/disassembler...
runtime/vm/disassembler_arm64.cc:1208: if (instr->SFField() == 0) {
Remove this if() {} and just use the format string below.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/disassembler...
runtime/vm/disassembler_arm64.cc:1217: Format(instr, "scvtfd 'vd, 'vn");
"scvtfd'sf 'vd, 'rn"

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/simulator_ar...
File runtime/vm/simulator_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm64.cc:2688: // Format(instr, "scvtfd 'vd, 'vn");
Fix comment to match change in disassembler.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm64.cc:2690: const double vn_dbl =
static_cast<double>(rn_val);
Instead of the if (instr->SFField() == 0) above:

const int64_t rn_val64 = get_register(rn, instr->RnMode());
const int32_t rn_val32 = get_wregister(rn, instr->RnMode());
const double vn_dbl = (instr->SFField() == 1) ?
    static_cast<double>(rn_val64) : static_cast<double>(rn_val32);

[Cutch, 2014-10-09 18:49:11]

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:722: void scvtfd(VRegister vd, Register rn) {
On 2014/10/09 18:00:30, zra wrote:
> scvtfdx to be consistent with x/w in other instructions.

Done.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:729: void scvtfd32(VRegister vd, Register rn) {
On 2014/10/09 18:00:30, zra wrote:
> scvtfdw to be consistent with x/w in other instructions.

Done.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:733: EmitFPIntCvtOp(SCVTFD32,
static_cast<Register>(vd), crn);
On 2014/10/09 18:00:30, zra wrote:
> Pass the size to EmitFPIntCvtOp instead of making a new opcode. Passing
> kDoubleWord sets B31, passing kWord doesn't. Remove B31 from the opcodes in
> FPIntCvtOp. This will make adding the 32-bit forms of the other instructions
> easier if it turns out we need them, as well.

Done.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/disassembler...
File runtime/vm/disassembler_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/disassembler...
runtime/vm/disassembler_arm64.cc:1208: if (instr->SFField() == 0) {
On 2014/10/09 18:00:30, zra wrote:
> Remove this if() {} and just use the format string below.

Done.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/disassembler...
runtime/vm/disassembler_arm64.cc:1217: Format(instr, "scvtfd 'vd, 'vn");
On 2014/10/09 18:00:30, zra wrote:
> "scvtfd'sf 'vd, 'rn"

Done.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/simulator_ar...
File runtime/vm/simulator_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm64.cc:2688: // Format(instr, "scvtfd 'vd, 'vn");
On 2014/10/09 18:00:30, zra wrote:
> Fix comment to match change in disassembler.

Done.

https://codereview.chromium.org/616873003/diff/140001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm64.cc:2690: const double vn_dbl =
static_cast<double>(rn_val);
On 2014/10/09 18:00:30, zra wrote:
> Instead of the if (instr->SFField() == 0) above:
> 
> const int64_t rn_val64 = get_register(rn, instr->RnMode());
> const int32_t rn_val32 = get_wregister(rn, instr->RnMode());
> const double vn_dbl = (instr->SFField() == 1) ?
>     static_cast<double>(rn_val64) : static_cast<double>(rn_val32);

Done.

[Vyacheslav Egorov (Google), 2014-10-09 19:08:58]

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.h:482: void cvtsi2sd32(XmmRegister a, Register b);
I'd think that ideally this instructions would use the same suffix schema as
other instructions.

s/cvtsi2sd/cvtsi2sdq/
s/cvtsi2sd32/cvtsi2sdl/

but maybe that would be too much changes around.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64_test.cc:2446: __ movq(RDX,
Immediate(0x2222222200000008));
maybe use another x64 register instead of RDX (e.g. R11 etc)

[zra, 2014-10-09 19:21:51]

arm64 lgtm with small fixes.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:1811: const int32_t sfield = (sz == kDoubleWord) ?
B31 : 0;
Assert that sz is either kDoubleWord or kWord since the other sizes don't make
sense.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/disassembler...
File runtime/vm/disassembler_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/disassembler...
runtime/vm/disassembler_arm64.cc:1283: Format(instr, "scvtfd'sf 'vd, 'vn");
'vn -> 'rn

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/simulator_ar...
File runtime/vm/simulator_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm64.cc:2766: if (instr->SFField() == 0) {
I'd merge this check with the condition at the top to keep the code in the
non-error case simpler.

|| ((instr->SFField() == 0) && (instr->Bits(16, 5) != 2))

[Cutch, 2014-10-09 20:48:23]

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:1811: const int32_t sfield = (sz == kDoubleWord) ?
B31 : 0;
On 2014/10/09 19:21:51, zra wrote:
> Assert that sz is either kDoubleWord or kWord since the other sizes don't make
> sense.

Done.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.h:482: void cvtsi2sd32(XmmRegister a, Register b);
On 2014/10/09 19:08:58, Vyacheslav Egorov (Google) wrote:
> I'd think that ideally this instructions would use the same suffix schema as
> other instructions.
> 
> s/cvtsi2sd/cvtsi2sdq/
> s/cvtsi2sd32/cvtsi2sdl/
> 
> but maybe that would be too much changes around. 

I had the same thought after Zach's suggestion. Done.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64_test.cc (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64_test.cc:2446: __ movq(RDX,
Immediate(0x2222222200000008));
On 2014/10/09 19:08:58, Vyacheslav Egorov (Google) wrote:
> maybe use another x64 register instead of RDX (e.g. R11 etc) 

Done.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/disassembler...
File runtime/vm/disassembler_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/disassembler...
runtime/vm/disassembler_arm64.cc:1283: Format(instr, "scvtfd'sf 'vd, 'vn");
On 2014/10/09 19:21:51, zra wrote:
> 'vn -> 'rn

Done.

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/simulator_ar...
File runtime/vm/simulator_arm64.cc (right):

https://codereview.chromium.org/616873003/diff/350001/runtime/vm/simulator_ar...
runtime/vm/simulator_arm64.cc:2766: if (instr->SFField() == 0) {
On 2014/10/09 19:21:51, zra wrote:
> I'd merge this check with the condition at the top to keep the code in the
> non-error case simpler.
> 
> || ((instr->SFField() == 0) && (instr->Bits(16, 5) != 2))

Done.

[Cutch, 2014-10-09 20:49:27]

Committed patchset #7 (id:280017) manually as 41025 (presubmit successful).

[Vyacheslav Egorov (Google), 2014-10-09 20:49:41]

LGTM