Revise assertions and fix bug in the implementation of shifts

Revise assertions and fix bug in the implementation of shifts

Previously failing assertions in the implementations of
ShiftMintOpInstr::EmitNativeCode for various architectures are revised:
* Incorrect assertion !has_shift_count_check() is removed
* Assertion for shift >= 0 is restored
* Assertion for shift < 64 on X64 is moved to kSHL branch

Also, misleading ShiftMintopInstr::has_shift_count_check() method
is replaced with method IsShiftCountInRange() with opposite meaning.

On X64, bug in the implementation of right shifts with constant shift
amount >= 64 is fixed. Corresponding test case is added.

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

Committed: https://github.com/dart-lang/sdk/commit/97f19892d4264f9102673d08d5f1cc9847a77550

[alexmarkov, 2017-07-11 18:22:32]

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

[regis, 2017-07-11 18:45:55]

lgtm

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

https://codereview.chromium.org/2974953002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:6067: if (CanDeoptimize()) {
When specifying --limit-ints-to-64-bits, we probably do not want to deoptimize,
but instead let the cpu do the operation, no? The behavior may not be the same
on all architectures, though. That is something to discuss and definitely not
for this CL. A TODO may be a good idea.

https://codereview.chromium.org/2974953002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:6103: // shrd is undefined with count >
operand size (32)
This last comment does not look correct? shrd with a 64-bit destination
considers 6 bits of the count.

[alexmarkov, 2017-07-11 19:39:24]

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

https://codereview.chromium.org/2974953002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:6067: if (CanDeoptimize()) {
On 2017/07/11 18:45:55, regis wrote:
> When specifying --limit-ints-to-64-bits, we probably do not want to
deoptimize,
> but instead let the cpu do the operation, no? The behavior may not be the same
> on all architectures, though. That is something to discuss and definitely not
> for this CL. A TODO may be a good idea.

Acknowledged.

https://codereview.chromium.org/2974953002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:6103: // shrd is undefined with count >
operand size (32)
On 2017/07/11 18:45:55, regis wrote:
> This last comment does not look correct? shrd with a 64-bit destination
> considers 6 bits of the count.

Done.

[zra, 2017-07-11 19:44:27]

lgtm

[alexmarkov, 2017-07-11 20:15:04]

Description was changed from

==========
Revise assertions and fix bug in the implementation of shifts

Previously failing assertions in the implementations of
ShiftMintOpInstr::EmitNativeCode for various architectures are revised:
* Incorrect assertion !has_shift_count_check() is removed
* Assertion for shift >= 0 is restored
* Assertion for shift < 64 on X64 is moved to kSHL branch

Also, misleading ShiftMintopInstr::has_shift_count_check() method
is replaced with method IsShiftCountInRange() with opposite meaning.

On X64, bug in the implementation of right shifts with constant shift
amount >= 64 is fixed. Corresponding test case is added.
==========

to

==========
Revise assertions and fix bug in the implementation of shifts

Previously failing assertions in the implementations of
ShiftMintOpInstr::EmitNativeCode for various architectures are revised:
* Incorrect assertion !has_shift_count_check() is removed
* Assertion for shift >= 0 is restored
* Assertion for shift < 64 on X64 is moved to kSHL branch

Also, misleading ShiftMintopInstr::has_shift_count_check() method
is replaced with method IsShiftCountInRange() with opposite meaning.

On X64, bug in the implementation of right shifts with constant shift
amount >= 64 is fixed. Corresponding test case is added.

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

Committed:
https://github.com/dart-lang/sdk/commit/97f19892d4264f9102673d08d5f1cc9847a77550
==========

[alexmarkov, 2017-07-11 20:15:06]

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

[Vyacheslav Egorov (Google), 2017-07-12 09:44:48]

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

[Vyacheslav Egorov (Google), 2017-07-12 09:44:48]

drive-by

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

https://codereview.chromium.org/2974953002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6548:
reinterpret_cast<int32_t>(locs()->in(1).constant().raw()) >> 1;
this should better be

Smi::Cast().Value()

instead of such manual untagging.

[alexmarkov, 2017-07-12 16:47:36]

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

https://codereview.chromium.org/2974953002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6548:
reinterpret_cast<int32_t>(locs()->in(1).constant().raw()) >> 1;
On 2017/07/12 09:44:48, Vyacheslav Egorov (Google) wrote:
> this should better be
> 
> Smi::Cast().Value()
> 
> instead of such manual untagging. 

Done, in the follow-up CL https://codereview.chromium.org/2975073002/