[turbofan] Merge sar/shr into MulHigh on ARM64

[turbofan] Merge sar/shr into MulHigh on ARM64

Merge a following arithmetic or logical right shift into the existing shift
of ARM64's Int32MulHigh or Uint32MulHigh code.

BUG=

Committed: https://crrev.com/1f876f245a3100dd4f51a12f28a4958fb92ece80
Cr-Commit-Position: refs/heads/master@{#28945}

[martyn.capewell, 2015-06-10 13:15:04]

martyn.capewell@arm.com changed reviewers:
+ bmeurer@chromium.org, ulan@chromium.org

[Benedikt Meurer, 2015-06-11 04:25:52]

https://codereview.chromium.org/1179503003/diff/1/src/compiler/arm64/instruct...
File src/compiler/arm64/instruction-selector-arm64.cc (right):

https://codereview.chromium.org/1179503003/diff/1/src/compiler/arm64/instruct...
src/compiler/arm64/instruction-selector-arm64.cc:749: if
(m.left().IsUint32MulHigh() && m.right().HasValue() &&
Can we limit the values on the right to [1,31] here? I.e. use
m.right().IsInRange(1,31) instead?

[martyn.capewell, 2015-06-11 10:01:20]

https://codereview.chromium.org/1179503003/diff/1/src/compiler/arm64/instruct...
File src/compiler/arm64/instruction-selector-arm64.cc (right):

https://codereview.chromium.org/1179503003/diff/1/src/compiler/arm64/instruct...
src/compiler/arm64/instruction-selector-arm64.cc:749: if
(m.left().IsUint32MulHigh() && m.right().HasValue() &&
On 2015/06/11 04:25:52, Benedikt Meurer wrote:
> Can we limit the values on the right to [1,31] here? I.e. use
> m.right().IsInRange(1,31) instead? 

I can, but I thought this operation was defined as only considering the bottom
five bits of the shift operand, as in JS. The unit test provided tries to check
this, too.

We also have a follow-up patch that resolves some of the semantics in this area.
For example, we've seen shifts generated with shift operands > 31, where the
operand is placed into a register for a register-controlled shift, which isn't
necessary.

[Benedikt Meurer, 2015-06-11 10:25:25]

On 2015/06/11 10:01:20, martyn.capewell wrote:
>
https://codereview.chromium.org/1179503003/diff/1/src/compiler/arm64/instruct...
> File src/compiler/arm64/instruction-selector-arm64.cc (right):
> 
>
https://codereview.chromium.org/1179503003/diff/1/src/compiler/arm64/instruct...
> src/compiler/arm64/instruction-selector-arm64.cc:749: if
> (m.left().IsUint32MulHigh() && m.right().HasValue() &&
> On 2015/06/11 04:25:52, Benedikt Meurer wrote:
> > Can we limit the values on the right to [1,31] here? I.e. use
> > m.right().IsInRange(1,31) instead? 
> 
> I can, but I thought this operation was defined as only considering the bottom
> five bits of the shift operand, as in JS. The unit test provided tries to
check
> this, too.
> 
> We also have a follow-up patch that resolves some of the semantics in this
area.
> For example, we've seen shifts generated with shift operands > 31, where the
> operand is placed into a register for a register-controlled shift, which isn't
> necessary.

Well, I'd prefer to keep it simple on this level. Shift operands outside the
valid range is undefined behavior in C/C++ and I'd like to adhere to that model
if possible. At least I'd be nice if we would not try to cover that with
optimizations.

[jbramley, 2015-06-11 13:11:27]

On 2015/06/11 10:01:20, martyn.capewell wrote:
> We also have a follow-up patch that resolves some of the semantics in this
> area.

The patch that Martyn mentioned is in issue 1179733004, along with my reasoning.

[Benedikt Meurer, 2015-06-11 13:23:47]

As mentioned in 1179733004, the argumentation makes sense, so let's go for it.
LGTM

[martyn.capewell, 2015-06-11 13:57:24]

The CQ bit was checked by martyn.capewell@arm.com

[commit-bot: I haz the power, 2015-06-11 13:57:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179503003/1

[commit-bot: I haz the power, 2015-06-11 14:50:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-11 14:50:23]

Patchset 1 (id:??) landed as
https://crrev.com/1f876f245a3100dd4f51a12f28a4958fb92ece80
Cr-Commit-Position: refs/heads/master@{#28945}