[wasm] Int64Lowering of Word64Ror and Word64Rol.

[wasm] Int64Lowering of Word64Ror and Word64Rol.

R=titzer@chromium.org

Committed: https://crrev.com/bd4fb28ecd3bd931b7ccfc13d39d4827589a8a54
Cr-Commit-Position: refs/heads/master@{#35173}

[titzer, 2016-03-30 14:56:35]

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering.cc
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering...
src/compiler/int64-lowering.cc:554: case IrOpcode::kWord64Ror: {
The names in here are a bit confusing. Fixed have a variety of meanings.

Suggestions:

fixed_shift -> masked_shift (or even masked_shift5 to indicate 5 bits)
fixed_low -> low_input
fixed_high -> high_input

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering...
src/compiler/int64-lowering.cc:579: if (fixed_shift_value == 0) {
Can you move this case up and save the creation of the constants just above? You
can make it even clearer if you just check shift_value == 32.

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering...
src/compiler/int64-lowering.cc:601: Node* fixed_low =
This is probably going to generate some bad code. The reason is that there is
essentially a conditional swap for both the high and low inputs (if shift >=
32).

A better sequence, which is probably less code with no swaps, is to do something
like

if(masked_shift < 32) (rotate code with non-swapped inputs)
else (rotate code with swapped inputs)

That way there are no extra moves involved with resolving the phis.

[ahaas, 2016-03-31 14:17:13]

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering.cc
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering...
src/compiler/int64-lowering.cc:554: case IrOpcode::kWord64Ror: {
On 2016/03/30 at 14:56:35, titzer wrote:
> The names in here are a bit confusing. Fixed have a variety of meanings.
> 
> Suggestions:
> 
> fixed_shift -> masked_shift (or even masked_shift5 to indicate 5 bits)
> fixed_low -> low_input
> fixed_high -> high_input

Done.

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering...
src/compiler/int64-lowering.cc:579: if (fixed_shift_value == 0) {
On 2016/03/30 at 14:56:35, titzer wrote:
> Can you move this case up and save the creation of the constants just above?
You can make it even clearer if you just check shift_value == 32.

Done.

https://codereview.chromium.org/1843123002/diff/1/src/compiler/int64-lowering...
src/compiler/int64-lowering.cc:601: Node* fixed_low =
On 2016/03/30 at 14:56:35, titzer wrote:
> This is probably going to generate some bad code. The reason is that there is
essentially a conditional swap for both the high and low inputs (if shift >=
32).
> 
> A better sequence, which is probably less code with no swaps, is to do
something like
> 
> if(masked_shift < 32) (rotate code with non-swapped inputs)
> else (rotate code with swapped inputs)
> 
> That way there are no extra moves involved with resolving the phis.

I did a complete rework of the lowering. Word64Ror is now lowered to two
Word32Ror. This allows us to get rid of one diamond.

[titzer, 2016-03-31 16:56:22]

lgtm!

[ahaas, 2016-03-31 16:57:23]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2016-03-31 16:57:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843123002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843123002/20001

[commit-bot: I haz the power, 2016-03-31 17:04:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-31 17:05:46]

Description was changed from

==========
[wasm] Int64Lowering of Word64Ror and Word64Rol.

R=titzer@chromium.org
==========

to

==========
[wasm] Int64Lowering of Word64Ror and Word64Rol.

R=titzer@chromium.org

Committed: https://crrev.com/bd4fb28ecd3bd931b7ccfc13d39d4827589a8a54
Cr-Commit-Position: refs/heads/master@{#35173}
==========

[commit-bot: I haz the power, 2016-03-31 17:05:48]

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