Implements checks for 53-bit overflow for x64.

Implements checks for 53-bit overflow for x64.

R=srdjan@google.com

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

[srdjan, 2013-06-06 21:59:02]

LGTM

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

https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:2136: if
(FLAG_throw_on_javascript_int_overflow) {
I think that it should be ASSERT(overflow != NULL). There are cases where we can
guarantee that we do not deoptimize (e.g., no 64 bit overflow), but this does
not mean that the range cannot be > 53 bits.

[zra, 2013-06-06 22:08:51]

On 2013/06/06 21:59:02, srdjan wrote:
> LGTM
> 
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> File runtime/vm/intermediate_language_x64.cc (right):
> 
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> runtime/vm/intermediate_language_x64.cc:2136: if
> (FLAG_throw_on_javascript_int_overflow) {
> I think that it should be ASSERT(overflow != NULL). There are cases where we
can
> guarantee that we do not deoptimize (e.g., no 64 bit overflow), but this does
> not mean that the range cannot be > 53 bits.

So, if CanDeoptimize() is false, I should skip the check?

[Ivan Posva, 2013-06-06 22:10:18]

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

https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:2136: if
(FLAG_throw_on_javascript_int_overflow) {
On 2013/06/06 21:59:02, srdjan wrote:
> I think that it should be ASSERT(overflow != NULL). There are cases where we
can
> guarantee that we do not deoptimize (e.g., no 64 bit overflow), but this does
> not mean that the range cannot be > 53 bits.

Actually if that flag is set we should just assume we can always overflow and
pass the overflow label.

https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:2157: Label* deopt =
shift_left->CanDeoptimize() ?
For example here we should assume that we can always deopt if 53-bit
verification is enabled, no?

https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:2363: if (CanDeoptimize()) {
For example right here.

[zra, 2013-06-06 22:17:21]

Oh, okay. I was confused. I was assuming that if CanDeoptimize() was false, then
there was no deopt label. But based on your suggestion, I suppose that there is
a deopt label even if CanDeoptimize is false. Is that correct?

On 2013/06/06 22:10:18, Ivan Posva wrote:
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> File runtime/vm/intermediate_language_x64.cc (right):
> 
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> runtime/vm/intermediate_language_x64.cc:2136: if
> (FLAG_throw_on_javascript_int_overflow) {
> On 2013/06/06 21:59:02, srdjan wrote:
> > I think that it should be ASSERT(overflow != NULL). There are cases where we
> can
> > guarantee that we do not deoptimize (e.g., no 64 bit overflow), but this
does
> > not mean that the range cannot be > 53 bits.
> 
> Actually if that flag is set we should just assume we can always overflow and
> pass the overflow label.
> 
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> runtime/vm/intermediate_language_x64.cc:2157: Label* deopt =
> shift_left->CanDeoptimize() ?
> For example here we should assume that we can always deopt if 53-bit
> verification is enabled, no?
> 
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> runtime/vm/intermediate_language_x64.cc:2363: if (CanDeoptimize()) {
> For example right here.

[srdjan, 2013-06-06 22:18:21]

On 2013/06/06 22:08:51, zra wrote:
> On 2013/06/06 21:59:02, srdjan wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> > File runtime/vm/intermediate_language_x64.cc (right):
> > 
> >
>
https://codereview.chromium.org/16398008/diff/1/runtime/vm/intermediate_langu...
> > runtime/vm/intermediate_language_x64.cc:2136: if
> > (FLAG_throw_on_javascript_int_overflow) {
> > I think that it should be ASSERT(overflow != NULL). There are cases where we
> can
> > guarantee that we do not deoptimize (e.g., no 64 bit overflow), but this
does
> > not mean that the range cannot be > 53 bits.
> 
> So, if CanDeoptimize() is false, I should skip the check?

The simplest solution is to change the appropriate CanDeoptimize to true if
53-bit mode is running and ASSERT that label is always non-null.
More elaborate optimization is probably unnecessary.

[zra, 2013-06-06 22:32:20]

On 2013/06/06 22:18:21, srdjan wrote:
> The simplest solution is to change the appropriate CanDeoptimize to true if
> 53-bit mode is running and ASSERT that label is always non-null.
> More elaborate optimization is probably unnecessary.

Done.

[srdjan, 2013-06-06 22:46:31]

lgtm

[zra, 2013-06-06 22:48:25]

Committed patchset #2 manually as r23721 (presubmit successful).

[Florian Schneider, 2013-06-07 07:33:15]

https://codereview.chromium.org/16398008/diff/10001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/16398008/diff/10001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:3699: __ j(OVERFLOW, deopt);
What about this operation?

https://codereview.chromium.org/16398008/diff/10001/tests/standalone/standalo...
File tests/standalone/standalone.status (left):

https://codereview.chromium.org/16398008/diff/10001/tests/standalone/standalo...
tests/standalone/standalone.status:161: 53bit_overflow_test: Fail
This test does not cover the generated code for smi-ops because it never
triggers optimization for the test functions (like integer_add).