Option to truncate integers to 64 bits, part 1 (core VM changes)

Option to truncate integers to 64 bits, part 1 (core VM changes)

This changeset revises --limit-ints-to-64-bits option to
change range of integers from unlimited to int64.
On overflow, integer arithmetic operations silently wrap around and
discard extra bits. No Bigints are allocated in case of overflow.

This changeset is the 1st part in the series of changes, it revises the implementation of integer arithmetic operations. More changes will follow.

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

Issue: https://github.com/dart-lang/sdk/issues/30103
Committed: https://github.com/dart-lang/sdk/commit/7a20b6b8d89ec22b9e036d17abba54ad52ef451c

[alexmarkov, 2017-07-07 18:32:08]

alexmarkov@google.com changed reviewers:
+ asiva@google.com, regis@google.com, zra@google.com

[regis, 2017-07-07 20:10:30]

https://codereview.chromium.org/2974633003/diff/1/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/lib/double.cc#newco...
runtime/lib/double.cc:84: // in truncating mode.
Something to discuss with the API people. You could pass a bool here indicating
whether to throw or to use min/max. I think it is safer to throw.

https://codereview.chromium.org/2974633003/diff/1/runtime/lib/double.cc#newco...
runtime/lib/double.cc:90: } else {  // representable in int64_t
The convention is to use a capital letter and a period:
// Representable as int64_t.

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
File runtime/platform/utils.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
type.
What does UB stand for?

https://codereview.chromium.org/2974633003/diff/1/runtime/tests/vm/dart/trunc...
File runtime/tests/vm/dart/truncating_ints_test.dart (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/tests/vm/dart/trunc...
runtime/tests/vm/dart/truncating_ints_test.dart:7: //
VMOptions=--truncate-ints-to-64-bits --enable-inlining-annotations
--optimization_counter_threshold=10 --no-use-osr --no-background-compilation
We usually write VMOptions just after the copyright and before comments.

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

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/flag_list.h#newc...
runtime/vm/flag_list.h:97: R(limit_ints_to_64_bits, false, bool, false,         
                       \
Do we need to keep this flag? Having both is somewhat confusing.
Or we should name this one more accurately, like
"throw_on_ints_larger_than_64_bits".

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:7195: #if defined(TARGET_ARCH_X64)
What about TARGET_ARCH_ARM64?

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

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19004: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
I think you should throw now or it will be difficult to test this cl.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19022: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
ditto

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19044: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
ditto

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19755: // TODO(alexmarkov): Throw error or assert in
--truncate-ints-to-64-bits mode.
We will keep these constructors for the future BigInt library. A throw or assert
would only be temporary for your own debugging but should not be inserted here.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19779: // TODO(alexmarkov): Throw error or assert in
--truncate-ints-to-64-bits mode.
ditto here and for all instances below.

[alexmarkov, 2017-07-07 20:46:39]

https://codereview.chromium.org/2974633003/diff/1/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/lib/double.cc#newco...
runtime/lib/double.cc:84: // in truncating mode.
On 2017/07/07 20:10:29, regis wrote:
> Something to discuss with the API people. You could pass a bool here
indicating
> whether to throw or to use min/max. I think it is safer to throw.

Acknowledged.

https://codereview.chromium.org/2974633003/diff/1/runtime/lib/double.cc#newco...
runtime/lib/double.cc:90: } else {  // representable in int64_t
On 2017/07/07 20:10:29, regis wrote:
> The convention is to use a capital letter and a period:
> // Representable as int64_t.

Done.

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
File runtime/platform/utils.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
type.
On 2017/07/07 20:10:30, regis wrote:
> What does UB stand for?

UB = Undefined Behavior. Do you think I should avoid this acronym?

https://codereview.chromium.org/2974633003/diff/1/runtime/tests/vm/dart/trunc...
File runtime/tests/vm/dart/truncating_ints_test.dart (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/tests/vm/dart/trunc...
runtime/tests/vm/dart/truncating_ints_test.dart:7: //
VMOptions=--truncate-ints-to-64-bits --enable-inlining-annotations
--optimization_counter_threshold=10 --no-use-osr --no-background-compilation
On 2017/07/07 20:10:30, regis wrote:
> We usually write VMOptions just after the copyright and before comments.

Done.

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

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/flag_list.h#newc...
runtime/vm/flag_list.h:97: R(limit_ints_to_64_bits, false, bool, false,         
                       \
On 2017/07/07 20:10:30, regis wrote:
> Do we need to keep this flag? Having both is somewhat confusing.
> Or we should name this one more accurately, like
> "throw_on_ints_larger_than_64_bits".

If you think it is no longer useful, I'll remove it in the future CLs, when I
will revise Integer/Bigint construction and add errors for huge literals (I left
TODOs for them).
Is that OK with you?

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:7195: #if defined(TARGET_ARCH_X64)
On 2017/07/07 20:10:30, regis wrote:
> What about TARGET_ARCH_ARM64?

As far as I can see from intermediate_language_arm64.cc, ARM64 does not support
unboxed Mints (inline code generation of operations with Mints). So, on ARM64
BinaryMintOpInstr is never used.

We might want to revise this in future. In such case, we will need to change
ComputeCanDeoptimize() according to the actual implementation of
BinaryMintOpInstr::EmitNativeCode() for ARM64.

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

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19004: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
On 2017/07/07 20:10:30, regis wrote:
> I think you should throw now or it will be difficult to test this cl.

Throwing error will require modifying parser and library (Dart) code as well.
I'd like to decouple those changes from changes in the semantics of arithmetic
operations.

Note that literals which fit into int64 and wrap-around arithmetic work just
fine with this CL - the corresponding test is included into CL.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19022: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
On 2017/07/07 20:10:30, regis wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19044: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
On 2017/07/07 20:10:30, regis wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19755: // TODO(alexmarkov): Throw error or assert in
--truncate-ints-to-64-bits mode.
On 2017/07/07 20:10:30, regis wrote:
> We will keep these constructors for the future BigInt library. A throw or
assert
> would only be temporary for your own debugging but should not be inserted
here. 

Acknowledged.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19779: // TODO(alexmarkov): Throw error or assert in
--truncate-ints-to-64-bits mode.
On 2017/07/07 20:10:30, regis wrote:
> ditto here and for all instances below.

Acknowledged.

[regis, 2017-07-07 21:46:53]

lgtm

But maybe Siva and/or Zach should have a look too.

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
File runtime/platform/utils.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
type.
On 2017/07/07 20:46:38, alexmarkov wrote:
> On 2017/07/07 20:10:30, regis wrote:
> > What does UB stand for?
> 
> UB = Undefined Behavior. Do you think I should avoid this acronym?

You can spell it out for clarity. I did not know UB.

Now, these operations on unsigned operands only produce the correct result if an
overflow actually occurs. Should we assert that the signed operands create an
overflow? Maybe not necessary.
Example: if you multiply -1 by +1, the result will be +1, which is not correct
(no overflow). But the product of kMaxInt64 by -2 is -2, which is correct
because of the overflow.

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

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/flag_list.h#newc...
runtime/vm/flag_list.h:97: R(limit_ints_to_64_bits, false, bool, false,         
                       \
On 2017/07/07 20:46:38, alexmarkov wrote:
> On 2017/07/07 20:10:30, regis wrote:
> > Do we need to keep this flag? Having both is somewhat confusing.
> > Or we should name this one more accurately, like
> > "throw_on_ints_larger_than_64_bits".
> 
> If you think it is no longer useful, I'll remove it in the future CLs, when I
> will revise Integer/Bigint construction and add errors for huge literals (I
left
> TODOs for them).
> Is that OK with you?

It is OK to keep it for now, but I think we need to rename it to make it clear
that it throws.

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:7195: #if defined(TARGET_ARCH_X64)
On 2017/07/07 20:46:38, alexmarkov wrote:
> On 2017/07/07 20:10:30, regis wrote:
> > What about TARGET_ARCH_ARM64?
> 
> As far as I can see from intermediate_language_arm64.cc, ARM64 does not
support
> unboxed Mints (inline code generation of operations with Mints). So, on ARM64
> BinaryMintOpInstr is never used.
> 
> We might want to revise this in future. In such case, we will need to change
> ComputeCanDeoptimize() according to the actual implementation of
> BinaryMintOpInstr::EmitNativeCode() for ARM64.

I was not aware of that. You should probably write a comment explaining it, or
readers will have the same question.

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

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:19004: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
On 2017/07/07 20:46:39, alexmarkov wrote:
> On 2017/07/07 20:10:30, regis wrote:
> > I think you should throw now or it will be difficult to test this cl.
> 
> Throwing error will require modifying parser and library (Dart) code as well.
> I'd like to decouple those changes from changes in the semantics of arithmetic
> operations.
> 
> Note that literals which fit into int64 and wrap-around arithmetic work just
> fine with this CL - the corresponding test is included into CL.

OK.

[alexmarkov, 2017-07-07 22:28:44]

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
File runtime/platform/utils.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
type.
On 2017/07/07 21:46:52, regis wrote:
> On 2017/07/07 20:46:38, alexmarkov wrote:
> > On 2017/07/07 20:10:30, regis wrote:
> > > What does UB stand for?
> > 
> > UB = Undefined Behavior. Do you think I should avoid this acronym?
> 
> You can spell it out for clarity. I did not know UB.
> 
> Now, these operations on unsigned operands only produce the correct result if
an
> overflow actually occurs. Should we assert that the signed operands create an
> overflow? Maybe not necessary.
> Example: if you multiply -1 by +1, the result will be +1, which is not correct
> (no overflow). But the product of kMaxInt64 by -2 is -2, which is correct
> because of the overflow. 

Changed UB to "undefined behavior".

As far as I know, result should be correct regardless of the overflow (on all
systems which use two's complement arithmetic).

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:7195: #if defined(TARGET_ARCH_X64)
On 2017/07/07 21:46:53, regis wrote:
> On 2017/07/07 20:46:38, alexmarkov wrote:
> > On 2017/07/07 20:10:30, regis wrote:
> > > What about TARGET_ARCH_ARM64?
> > 
> > As far as I can see from intermediate_language_arm64.cc, ARM64 does not
> support
> > unboxed Mints (inline code generation of operations with Mints). So, on
ARM64
> > BinaryMintOpInstr is never used.
> > 
> > We might want to revise this in future. In such case, we will need to change
> > ComputeCanDeoptimize() according to the actual implementation of
> > BinaryMintOpInstr::EmitNativeCode() for ARM64.
> 
> I was not aware of that. You should probably write a comment explaining it, or
> readers will have the same question.

Done.

[regis, 2017-07-07 22:38:25]

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
File runtime/platform/utils.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
type.
On 2017/07/07 22:28:44, alexmarkov wrote:
> On 2017/07/07 21:46:52, regis wrote:
> > On 2017/07/07 20:46:38, alexmarkov wrote:
> > > On 2017/07/07 20:10:30, regis wrote:
> > > > What does UB stand for?
> > > 
> > > UB = Undefined Behavior. Do you think I should avoid this acronym?
> > 
> > You can spell it out for clarity. I did not know UB.
> > 
> > Now, these operations on unsigned operands only produce the correct result
if
> an
> > overflow actually occurs. Should we assert that the signed operands create
an
> > overflow? Maybe not necessary.
> > Example: if you multiply -1 by +1, the result will be +1, which is not
correct
> > (no overflow). But the product of kMaxInt64 by -2 is -2, which is correct
> > because of the overflow. 
> 
> Changed UB to "undefined behavior".
> 
> As far as I know, result should be correct regardless of the overflow (on all
> systems which use two's complement arithmetic).

Two's complement arithmetic can ignore the sign for addition and subtraction,
but not for multiplication and division. The sign can be ignored only if an
overflow occurs, i.e. the sign bit is at position > 63.
As mentioned, -1 * +1 should be -1, not +1.

[siva, 2017-07-07 23:11:21]

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/double.cc#n...
runtime/lib/double.cc:120: }
Why not add your new code here
if (Flag_...) {
  if (val <= static_cast<double>(kMinInt64)) {
    ival = kMinInt64;
  } else {
    ASSERT(ival >= static_cast<double>(kMaxInt64));
    ival = kMaxInt64;
  }
  return Integer::New(ival);
}

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/integers.cc...
runtime/lib/integers.cc:417: if (FLAG_limit_ints_to_64_bits) {
Not sure I understand why we should have two flags
--limit_ints_to_64_bits and --truncate_ints_to_64_bits

https://codereview.chromium.org/2974633003/diff/40001/runtime/tests/vm/dart/t...
File runtime/tests/vm/dart/truncating_ints_test.dart (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/tests/vm/dart/t...
runtime/tests/vm/dart/truncating_ints_test.dart:5: //
VMOptions=--truncate-ints-to-64-bits --enable-inlining-annotations
--optimization_counter_threshold=10 --no-use-osr --no-background-compilation
Why is the option --no-use-osr significant here?

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

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/flag_list.h#...
runtime/vm/flag_list.h:100: "Truncate integers to 64 bits")                     
                      \
Why do we want two flags limit_ints_to_64_bits and truncate_ints_to_64_bits, why
not just combine the two flags into one say int_is_int64.

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1821: result = Integer::New(truncated,
Heap::kOld);
Why is it necessary to allocate in old space? the value seems to be
canonicalized below which will ensure it gets to the old space.

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

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19004: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
You have this comment here and below of throwing an error, is this because of
potential used of these methods from the parser?

[alexmarkov, 2017-07-07 23:17:58]

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
File runtime/platform/utils.h (right):

https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
type.
On 2017/07/07 22:38:25, regis wrote:
> On 2017/07/07 22:28:44, alexmarkov wrote:
> > On 2017/07/07 21:46:52, regis wrote:
> > > On 2017/07/07 20:46:38, alexmarkov wrote:
> > > > On 2017/07/07 20:10:30, regis wrote:
> > > > > What does UB stand for?
> > > > 
> > > > UB = Undefined Behavior. Do you think I should avoid this acronym?
> > > 
> > > You can spell it out for clarity. I did not know UB.
> > > 
> > > Now, these operations on unsigned operands only produce the correct result
> if
> > an
> > > overflow actually occurs. Should we assert that the signed operands create
> an
> > > overflow? Maybe not necessary.
> > > Example: if you multiply -1 by +1, the result will be +1, which is not
> correct
> > > (no overflow). But the product of kMaxInt64 by -2 is -2, which is correct
> > > because of the overflow. 
> > 
> > Changed UB to "undefined behavior".
> > 
> > As far as I know, result should be correct regardless of the overflow (on
all
> > systems which use two's complement arithmetic).
> 
> Two's complement arithmetic can ignore the sign for addition and subtraction,
> but not for multiplication and division. The sign can be ignored only if an
> overflow occurs, i.e. the sign bit is at position > 63.
> As mentioned, -1 * +1 should be -1, not +1.

I agree about division, but still curious about multiplication.

If the high (64+) bits of the multiplication result are discarded, the low bits
(0..63) of the result should be the same regardless of signed or unsigned
multiplication (for the arguments with the same bit patterns). Note that sign
bit is not ignored - it is still taken into account during unsigned
multiplication.

In this particular case,
static_cast<uint64_t>(-1) = 0xffffffffffffffff
static_cast<uint64_t>(1) = 1
static_cast<uint64_t>(-1) * static_cast<uint64_t>(1) = 0xffffffffffffffff
static_cast<int64_t>(static_cast<uint64_t>(-1) * static_cast<uint64_t>(1)) = -1

So MulWithWrapAround(-1, 1) = -1, which looks correct.

Could you please clarify your doubts a little bit more?

[regis, 2017-07-07 23:24:56]

On 2017/07/07 23:17:58, alexmarkov wrote:
> https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h
> File runtime/platform/utils.h (right):
> 
>
https://codereview.chromium.org/2974633003/diff/1/runtime/platform/utils.h#ne...
> runtime/platform/utils.h:190: // Avoid UB by doing arithmetic in the unsigned
> type.
> On 2017/07/07 22:38:25, regis wrote:
> > On 2017/07/07 22:28:44, alexmarkov wrote:
> > > On 2017/07/07 21:46:52, regis wrote:
> > > > On 2017/07/07 20:46:38, alexmarkov wrote:
> > > > > On 2017/07/07 20:10:30, regis wrote:
> > > > > > What does UB stand for?
> > > > > 
> > > > > UB = Undefined Behavior. Do you think I should avoid this acronym?
> > > > 
> > > > You can spell it out for clarity. I did not know UB.
> > > > 
> > > > Now, these operations on unsigned operands only produce the correct
result
> > if
> > > an
> > > > overflow actually occurs. Should we assert that the signed operands
create
> > an
> > > > overflow? Maybe not necessary.
> > > > Example: if you multiply -1 by +1, the result will be +1, which is not
> > correct
> > > > (no overflow). But the product of kMaxInt64 by -2 is -2, which is
correct
> > > > because of the overflow. 
> > > 
> > > Changed UB to "undefined behavior".
> > > 
> > > As far as I know, result should be correct regardless of the overflow (on
> all
> > > systems which use two's complement arithmetic).
> > 
> > Two's complement arithmetic can ignore the sign for addition and
subtraction,
> > but not for multiplication and division. The sign can be ignored only if an
> > overflow occurs, i.e. the sign bit is at position > 63.
> > As mentioned, -1 * +1 should be -1, not +1.
> 
> I agree about division, but still curious about multiplication.
> 
> If the high (64+) bits of the multiplication result are discarded, the low
bits
> (0..63) of the result should be the same regardless of signed or unsigned
> multiplication (for the arguments with the same bit patterns). Note that sign
> bit is not ignored - it is still taken into account during unsigned
> multiplication.
> 
> In this particular case,
> static_cast<uint64_t>(-1) = 0xffffffffffffffff
> static_cast<uint64_t>(1) = 1
> static_cast<uint64_t>(-1) * static_cast<uint64_t>(1) = 0xffffffffffffffff
> static_cast<int64_t>(static_cast<uint64_t>(-1) * static_cast<uint64_t>(1)) =
-1
> 
> So MulWithWrapAround(-1, 1) = -1, which looks correct.
> 
> Could you please clarify your doubts a little bit more?

Sorry, you are correct. It makes no difference.
In my mind, static_cast<uint64_t>(-1) would change the bit pattern to +1, but
that is not the case, of course. Sorry for wasting your time :-)

[alexmarkov, 2017-07-10 16:56:52]

Description was changed from

==========
Option to truncate integers to 64 bits, part 1 (core VM changes)

This changeset introduces --truncate-ints-to-64-bits option which
changes range of integers from unlimited to int64.
On overflow, integer arithmetic operations silently wrap around and
discard extra bits. No Bigints are allocated in case of overflow.

This changeset is the 1st part in the series of changes, it adds
support for the new option in the implementation of integer arithmetic
operations. More changes will follow.

Issue: https://github.com/dart-lang/sdk/issues/30103
==========

to

==========
Option to truncate integers to 64 bits, part 1 (core VM changes)

This changeset revises --limit-ints-to-64-bits option to
change range of integers from unlimited to int64.
On overflow, integer arithmetic operations silently wrap around and
discard extra bits. No Bigints are allocated in case of overflow.

This changeset is the 1st part in the series of changes, it revises the
implementation of integer arithmetic operations. More changes will follow.

Issue: https://github.com/dart-lang/sdk/issues/30103
==========

[alexmarkov, 2017-07-10 16:58:13]

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/double.cc#n...
runtime/lib/double.cc:120: }
On 2017/07/07 23:11:20, siva wrote:
> Why not add your new code here
> if (Flag_...) {
>   if (val <= static_cast<double>(kMinInt64)) {
>     ival = kMinInt64;
>   } else {
>     ASSERT(ival >= static_cast<double>(kMaxInt64));
>     ival = kMaxInt64;
>   }
>   return Integer::New(ival);
> }

Although the code up to this points works fine in truncating mode, it is
unnecessary to look into double internals in truncating mode - generic code
"static_cast<int64_t>(val)" covers it just fine.

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/lib/integers.cc...
runtime/lib/integers.cc:417: if (FLAG_limit_ints_to_64_bits) {
On 2017/07/07 23:11:20, siva wrote:
> Not sure I understand why we should have two flags
> --limit_ints_to_64_bits and --truncate_ints_to_64_bits

Done.

https://codereview.chromium.org/2974633003/diff/40001/runtime/tests/vm/dart/t...
File runtime/tests/vm/dart/truncating_ints_test.dart (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/tests/vm/dart/t...
runtime/tests/vm/dart/truncating_ints_test.dart:5: //
VMOptions=--truncate-ints-to-64-bits --enable-inlining-annotations
--optimization_counter_threshold=10 --no-use-osr --no-background-compilation
On 2017/07/07 23:11:20, siva wrote:
> Why is the option --no-use-osr significant here?

It can be omitted. However, it helps to avoid re-compilation of code which
verifies the results of operations (test_add, test_sub, test_mul and test_shl),
while the function with operation is re-compiled. It ensures that the same code,
without any changes, verifies the results of both non-optimized and optimized
operations.

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

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/flag_list.h#...
runtime/vm/flag_list.h:100: "Truncate integers to 64 bits")                     
                      \
On 2017/07/07 23:11:20, siva wrote:
> Why do we want two flags limit_ints_to_64_bits and truncate_ints_to_64_bits,
why
> not just combine the two flags into one say int_is_int64.

Done.

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1821: result = Integer::New(truncated,
Heap::kOld);
On 2017/07/07 23:11:20, siva wrote:
> Why is it necessary to allocate in old space? the value seems to be
> canonicalized below which will ensure it gets to the old space.

It is a bug fix. This code is used to do constant folding in the background
compilation thread, which is unable to allocate objects in the young generation
(causes assertion failure).

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

https://codereview.chromium.org/2974633003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19004: // TODO(alexmarkov): Throw error in
FLAG_truncate_ints_to_64_bits mode.
On 2017/07/07 23:11:20, siva wrote:
> You have this comment here and below of throwing an error, is this because of
> potential used of these methods from the parser?

Yes.

[zra, 2017-07-10 21:54:03]

lgtm

https://codereview.chromium.org/2974633003/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2974633003/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19755: // TODO(alexmarkov): Throw error or assert in
--limit-ints-to-64-bits mode.
... until there's something in the language spec or libraries that defines a
legal way to allocate them.

[alexmarkov, 2017-07-10 22:16:45]

Description was changed from

==========
Option to truncate integers to 64 bits, part 1 (core VM changes)

This changeset revises --limit-ints-to-64-bits option to
change range of integers from unlimited to int64.
On overflow, integer arithmetic operations silently wrap around and
discard extra bits. No Bigints are allocated in case of overflow.

This changeset is the 1st part in the series of changes, it revises the
implementation of integer arithmetic operations. More changes will follow.

Issue: https://github.com/dart-lang/sdk/issues/30103
==========

to

==========
Option to truncate integers to 64 bits, part 1 (core VM changes)

This changeset revises --limit-ints-to-64-bits option to
change range of integers from unlimited to int64.
On overflow, integer arithmetic operations silently wrap around and
discard extra bits. No Bigints are allocated in case of overflow.

This changeset is the 1st part in the series of changes, it revises the
implementation of integer arithmetic operations. More changes will follow.

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

Issue: https://github.com/dart-lang/sdk/issues/30103
Committed:
https://github.com/dart-lang/sdk/commit/7a20b6b8d89ec22b9e036d17abba54ad52ef451c
==========

[alexmarkov, 2017-07-10 22:16:47]

Committed patchset #4 (id:60001) manually as
7a20b6b8d89ec22b9e036d17abba54ad52ef451c (presubmit successful).

[alexmarkov, 2017-07-10 22:21:54]

https://codereview.chromium.org/2974633003/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2974633003/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19755: // TODO(alexmarkov): Throw error or assert in
--limit-ints-to-64-bits mode.
On 2017/07/10 21:54:03, zra wrote:
> ... until there's something in the language spec or libraries that defines a
> legal way to allocate them.

Acknowledged.