Fix StoreIndexedInstr input representation requirements for Int32/Uint32 arrays.

Fix StoreIndexedInstr input representation requirements for Int32/Uint32 arrays.

Previous implementation changed input representation depending on the propagated type of the value which violated assumptions made by SelectRepresentations phase.

Instead of using tagged/mint input require unboxed Int32/Uint32 input and insert explicit truncating unboxing when building StoreIndexed operation in the optimizer. This also leads to strictly better code and opens possibilities for further optimizations.

Implement Int32/Uint32 representation support on all platforms. This includes boxing, unboxing and unboxed converter operations.

Merge BoxInt32/BoxUint32 and UnboxInt32/UnboxUint32 instruction sequences to minimize duplication.

Improve instruction sequences by utilizing CARRY flag set by smi untagging where possible (ARM, ia32, x86).

Enable all tests that were disabled by r40078, r40079.

BUG=http://dartbug.com/20875
R=fschneider@google.com, johnmccutchan@google.com, srdjan@google.com, zra@google.com

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

[Vyacheslav Egorov (Google), 2014-09-10 21:47:51]

vegorov@google.com changed reviewers:
+ fschneider@google.com, johnmccutchan@google.com, srdjan@google.com,
zra@google.com

[Vyacheslav Egorov (Google), 2014-09-10 21:47:51]

PTAL

[Cutch, 2014-09-10 22:27:56]

lgtm

[srdjan, 2014-09-10 22:32:44]

LGTM

[zra, 2014-09-10 23:17:22]

lgtm

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.cc#...
runtime/vm/assembler_arm.cc:2267: if (shift_imm == 32) shift_imm = 0;  // Comply
to UAL syntax.
Please use curly braces.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.h#n...
runtime/vm/assembler_arm.h:720: void AsrS(Register rd, Register rm, uint32_t
shift_imm, Condition cond = AL);
It looks like we've been using a little s, i.e. Asrs, to indicate the version of
an instruction that sets condition codes.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.h#n...
runtime/vm/assembler_arm.h:750: void SmiUntag(Register dst, Register src, Label*
ok) {
Maybe comment here about what the Label is for.

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

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:8289: if (result.IsNull()) {
What was the solution to this?

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

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:1387: 
extra newline

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:1469: // mints (ia32 with at least SSE 4.1).
Update comment.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:3730: // generate Mint on 32-bit
architectures.
Maybe also indent comment.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_arm64.cc:5215: __ cmp(out, Operand(out, SXTW,
0));
Not sure if this helps you any, but if you specify the size in
LoadFieldFromOffset, i.e. LoadFieldFromOffset(..., PP, {kWord, kUnsignedWord}),
it will read 32 bits and then sign extend into the result register, always
extending 0's for kUnsignedWord no matter what the top bit is.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_x64.cc:5583: if (!is_truncating() && (deopt !=
NULL)) {
Maybe copy your comment from arm64 here as well?

[Florian Schneider, 2014-09-11 09:38:54]

Lgtm.

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1387: 
Remove extra \n.

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1469: // mints (ia32 with at least SSE 4.1).
This comment is now out of date.

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

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:6772: const intptr_t kNumTemps =
Simplify to 
kNumTemps = CanDeoptimize() ? 1 : 0;

[Vyacheslav Egorov (Google), 2014-09-11 11:49:17]

Thanks for the review!

Landing.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.cc#...
runtime/vm/assembler_arm.cc:2267: if (shift_imm == 32) shift_imm = 0;  // Comply
to UAL syntax.
On 2014/09/10 23:17:21, zra wrote:
> Please use curly braces.

Done.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.h#n...
runtime/vm/assembler_arm.h:720: void AsrS(Register rd, Register rm, uint32_t
shift_imm, Condition cond = AL);
On 2014/09/10 23:17:22, zra wrote:
> It looks like we've been using a little s, i.e. Asrs, to indicate the version
of
> an instruction that sets condition codes.

Done.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/assembler_arm.h#n...
runtime/vm/assembler_arm.h:750: void SmiUntag(Register dst, Register src, Label*
ok) {
On 2014/09/10 23:17:22, zra wrote:
> Maybe comment here about what the Label is for.

Done.

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

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:8289: if (result.IsNull()) {
On 2014/09/10 23:17:22, zra wrote:
> What was the solution to this?

I looked into the Smi::ShiftOp and found that it *never* returns Null.

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

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:1387: 
On 2014/09/10 23:17:22, zra wrote:
> extra newline

Done.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:1469: // mints (ia32 with at least SSE 4.1).
On 2014/09/10 23:17:22, zra wrote:
> Update comment.

Done.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:3730: // generate Mint on 32-bit
architectures.
On 2014/09/10 23:17:22, zra wrote:
> Maybe also indent comment.

Done.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_arm64.cc:5215: __ cmp(out, Operand(out, SXTW,
0));
On 2014/09/10 23:17:22, zra wrote:
> Not sure if this helps you any, but if you specify the size in
> LoadFieldFromOffset, i.e. LoadFieldFromOffset(..., PP, {kWord,
kUnsignedWord}),
> it will read 32 bits and then sign extend into the result register, always
> extending 0's for kUnsignedWord no matter what the top bit is.

I am doing int64 -> int32 convertion here. What I want to check is that this
conversion is lossless, e.g. that value was 32-bit signed value to begin with.
Essentially I am doing:

static_cast<int64_t>(static_cast<int32_t>(v)) == v

So I can't load it as a Word in !is_truncating() case. I could load it as a word
in is_truncating() case, but I decided against that for uniformity for now.

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/552303005/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_x64.cc:5583: if (!is_truncating() && (deopt !=
NULL)) {
On 2014/09/10 23:17:22, zra wrote:
> Maybe copy your comment from arm64 here as well?

Done.

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1387: 
On 2014/09/11 09:38:53, Florian Schneider wrote:
> Remove extra \n.

Done.

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1469: // mints (ia32 with at least SSE 4.1).
On 2014/09/11 09:38:53, Florian Schneider wrote:
> This comment is now out of date.

Done.

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

https://codereview.chromium.org/552303005/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:6772: const intptr_t kNumTemps =
On 2014/09/11 09:38:53, Florian Schneider wrote:
> Simplify to 
> kNumTemps = CanDeoptimize() ? 1 : 0;

Done.

[Vyacheslav Egorov (Google), 2014-09-11 12:33:16]

Committed patchset #2 (id:20001) manually as 40143 (presubmit successful).