Add unboxed Mint for X64

Add unboxed Mint for X64

BUG=
R=vegorov@google.com

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

[Cutch, 2014-10-03 23:39:38]

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

[Vyacheslav Egorov (Google), 2014-10-05 18:47:00]

initial comments

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

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:494: static Condition
TokenKindToMintCondition(Token::Kind kind) {
I don't think there is a need to have this duplicated. Just
s/SmiCondition/IntCondition/ and use that function.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:509: static Condition
EmitMintComparisonOp(FlowGraphCompiler* compiler,
This is essentially the same as EmitSmiComparisonOp. Can we use that?

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5536: __ testq(value,
Immediate(kSmiTagMask));
You can produce much smaller instruction sequence with (assuming out has
SameAsFirstInput policy).

    __ SmiUntagOrCheckClass(value, kMintCid, &done);
    __ j(NOT_EQUAL, deopt);
    // Undo untagging by multiplying value with 2.
    __ movq(value, Address(value, TIMES_2, Mint::value_offset()));
    __ Bind(&done);

See UnboxIntNInstr

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5579: __ SmiUntag(out);
__ movq(out, value);
__ SmiTag(out);   // shrl sets OF := SF ^ CF on 1 bit shifts
__ j(NOT_OVERFLOW, &done);
BoxAllocationSlowPath::Allocate(
    compiler, this, compiler->mint_class(), out);
__ movq(FieldAddress(out, Mint::value_offset()), value);
__ Bind(&done);

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5618: switch (op_kind()) {
use EmitIntegerArithmetic

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5725: if ((shift < 0) || (shift >
kMintShiftCountLimit)) {
These cases should be handled in the Canonicalize already.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5904: if (shift_value == 0) {
Should be handled in the Canonicalize

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:6128: __ movsxd(out, out);
you could do __ movsxd(out, value) to begin with.

if (!CanDeoptimize()) {
  __ movl(out, value);
} else {
  __ movsxd(out, value);
  __ cmpq(...);
  ...
}

[Cutch, 2014-10-06 23:55:26]

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

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:494: static Condition
TokenKindToMintCondition(Token::Kind kind) {
On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> I don't think there is a need to have this duplicated. Just
> s/SmiCondition/IntCondition/ and use that function.

Done.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:509: static Condition
EmitMintComparisonOp(FlowGraphCompiler* compiler,
On 2014/10/05 18:47:00, Vyacheslav Egorov (Google) wrote:
> This is essentially the same as EmitSmiComparisonOp. Can we use that?

Done.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5536: __ testq(value,
Immediate(kSmiTagMask));
On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> You can produce much smaller instruction sequence with (assuming out has
> SameAsFirstInput policy).
> 
>     __ SmiUntagOrCheckClass(value, kMintCid, &done);
>     __ j(NOT_EQUAL, deopt);
>     // Undo untagging by multiplying value with 2.
>     __ movq(value, Address(value, TIMES_2, Mint::value_offset()));
>     __ Bind(&done);
> 
> See UnboxIntNInstr 

Done.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5579: __ SmiUntag(out);
On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> __ movq(out, value);
> __ SmiTag(out);   // shrl sets OF := SF ^ CF on 1 bit shifts
> __ j(NOT_OVERFLOW, &done);
> BoxAllocationSlowPath::Allocate(
>     compiler, this, compiler->mint_class(), out);
> __ movq(FieldAddress(out, Mint::value_offset()), value);
> __ Bind(&done);

Done.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5618: switch (op_kind()) {
On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> use EmitIntegerArithmetic

EmitIntegerArithmetic does 32-bit operations, these are 64-it operations. I've
split this out into an EmitInteger64Arithmetic utility function.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5725: if ((shift < 0) || (shift >
kMintShiftCountLimit)) {
On 2014/10/05 18:47:00, Vyacheslav Egorov (Google) wrote:
> These cases should be handled in the Canonicalize already. 

Done.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5904: if (shift_value == 0) {
On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> Should be handled in the Canonicalize

Done.

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:6128: __ movsxd(out, out);
On 2014/10/05 18:47:00, Vyacheslav Egorov (Google) wrote:
> you could do __ movsxd(out, value) to begin with.
> 
> if (!CanDeoptimize()) {
>   __ movl(out, value);
> } else {
>   __ movsxd(out, value);
>   __ cmpq(...);
>   ...
> }

Done.

[Vyacheslav Egorov (Google), 2014-10-07 12:03:50]

lgtm

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

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5618: switch (op_kind()) {
On 2014/10/06 23:55:25, Cutch wrote:
> On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> > use EmitIntegerArithmetic
> 
> EmitIntegerArithmetic does 32-bit operations, these are 64-it operations. I've
> split this out into an EmitInteger64Arithmetic utility function.

Ack, true, shame on me! 

I was dwelling in ia32-thinking where Smi/(U)Int32 arithmetic can be unified. 

Here Mint/Smi can be unified instead.

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

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:6515: if (shift_value > kShifterLimit) {
Can't this also be handled in Canonicalize just like other cases?

If it's not handled yet, please just move there so that all platforms will
benefit.

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6134: if (shift_value > kShifterLimit)
{
Ditto.

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

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:470: static Condition
EmitIntComparisonOp(FlowGraphCompiler* compiler,
Maybe it's better to call it Int64ComparisonOp to reflect that it does cmpq.

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5537: __ SmiTag(out);   // shrl sets OF
:= SF ^ CF on 1 bit shifts
I had a typo in this comment, sorry. It's tagging on x64 so it's shlq

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5852: if (shift_value > kShifterLimit) {
Is this not handled in Canonicalize?

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:6072: __ movsxd(out, out);
__ movsxd(out, value)

[Cutch, 2014-10-21 17:42:12]

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

https://codereview.chromium.org/626223002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5618: switch (op_kind()) {
On 2014/10/07 12:03:50, Vyacheslav Egorov (Google) wrote:
> On 2014/10/06 23:55:25, Cutch wrote:
> > On 2014/10/05 18:46:59, Vyacheslav Egorov (Google) wrote:
> > > use EmitIntegerArithmetic
> > 
> > EmitIntegerArithmetic does 32-bit operations, these are 64-it operations.
I've
> > split this out into an EmitInteger64Arithmetic utility function.
> 
> Ack, true, shame on me! 
> 
> I was dwelling in ia32-thinking where Smi/(U)Int32 arithmetic can be unified. 
> 
> Here Mint/Smi can be unified instead.

Done.

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

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:6515: if (shift_value > kShifterLimit) {
On 2014/10/07 12:03:50, Vyacheslav Egorov (Google) wrote:
> Can't this also be handled in Canonicalize just like other cases?
> 
> If it's not handled yet, please just move there so that all platforms will
> benefit.

Done.

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6134: if (shift_value > kShifterLimit)
{
On 2014/10/07 12:03:50, Vyacheslav Egorov (Google) wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:5852: if (shift_value > kShifterLimit) {
On 2014/10/07 12:03:50, Vyacheslav Egorov (Google) wrote:
> Is this not handled in Canonicalize?

Done.

https://codereview.chromium.org/626223002/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:6072: __ movsxd(out, out);
On 2014/10/07 12:03:50, Vyacheslav Egorov (Google) wrote:
> __ movsxd(out, value)

Done.

[Cutch, 2014-10-21 18:12:12]

Committed patchset #5 (id:80001) manually as 41233 (presubmit successful).