SIMD shuffle API changes

SIMD shuffle API changes

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

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

[Cutch, 2013-10-30 05:58:13]

SIMD shuffle API changes:

1) Removes interleave* methods and replaces them with a more general shuffleMix
method.
2) Adds shuffle and shuffleMix to Uint32x4 class.

[zra, 2013-10-30 17:39:27]

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

https://codereview.chromium.org/51373004/diff/1/runtime/lib/simd128.cc#newcod...
runtime/lib/simd128.cc:285: GET_NON_NULL_NATIVE_ARGUMENT(Float32x4, zw,
arguments->NativeArgAt(1));
Just a bit confused about naming, I think. This is called zw, but could the
shuffleMix also take the x or y elements for the final result?

https://codereview.chromium.org/51373004/diff/1/runtime/lib/simd128.cc#newcod...
runtime/lib/simd128.cc:546: GET_NON_NULL_NATIVE_ARGUMENT(Uint32x4, zw,
arguments->NativeArgAt(1));
Same question here.

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

https://codereview.chromium.org/51373004/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:3143: case
MethodRecognizer::kUint32x4ShuffleMix:
Is it possible to have special cases where vdup can be used?

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1276: Float32x4
shuffleMix(Float32x4 other, int m) {
Maybe 'other' is also a good name for the argument in the VM code.

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1467: /// Mask passed to
[shuffle] and [shuffleMix].
Is there any way to refactor this so we only have one big list of constants?

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/typed_data...
File sdk/lib/typed_data/typed_data.dart (right):

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/typed_data...
sdk/lib/typed_data/typed_data.dart:1209: /// Mask passed to [shuffle] or
[shuffleMix].
Maybe refactor?

[Cutch, 2013-10-31 01:43:55]

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

https://codereview.chromium.org/51373004/diff/1/runtime/lib/simd128.cc#newcod...
runtime/lib/simd128.cc:285: GET_NON_NULL_NATIVE_ARGUMENT(Float32x4, zw,
arguments->NativeArgAt(1));
On 2013/10/30 17:39:27, zra wrote:
> Just a bit confused about naming, I think. This is called zw, but could the
> shuffleMix also take the x or y elements for the final result?


name of "zw" because the resulting ZW lanes will be pulled from it. Renamed it
to "other".

https://codereview.chromium.org/51373004/diff/1/runtime/lib/simd128.cc#newcod...
runtime/lib/simd128.cc:546: GET_NON_NULL_NATIVE_ARGUMENT(Uint32x4, zw,
arguments->NativeArgAt(1));
On 2013/10/30 17:39:27, zra wrote:
> Same question here.

Renamed to other.

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

https://codereview.chromium.org/51373004/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:3143: case
MethodRecognizer::kUint32x4ShuffleMix:
On 2013/10/30 17:39:27, zra wrote:
> Is it possible to have special cases where vdup can be used?

I'm not familiar enough with the NEON shuffling instructions, but, I expect all
shuffle and shuffleMix masks have better instruction sequences than the __ vmovs
code.

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1276: Float32x4
shuffleMix(Float32x4 other, int m) {
On 2013/10/30 17:39:27, zra wrote:
> Maybe 'other' is also a good name for the argument in the VM code.

Done.

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1467: /// Mask passed to
[shuffle] and [shuffleMix].
On 2013/10/30 17:39:27, zra wrote:
> Is there any way to refactor this so we only have one big list of constants?

Not without introducing a class that holds the shuffle constants (which I don't
want to do). static const members are not inherited through extend or with a
mixin— I tried that.

[zra, 2013-10-31 15:10:20]

lgtm

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

https://codereview.chromium.org/51373004/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:3143: case
MethodRecognizer::kUint32x4ShuffleMix:
On 2013/10/31 01:43:55, Cutch wrote:
> On 2013/10/30 17:39:27, zra wrote:
> > Is it possible to have special cases where vdup can be used?
> 
> I'm not familiar enough with the NEON shuffling instructions, but, I expect
all
> shuffle and shuffleMix masks have better instruction sequences than the __
vmovs
> code.

Please leave a TODO here for me, and I'll look into it.

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://codereview.chromium.org/51373004/diff/1/sdk/lib/typed_data/dart2js/ty...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1467: /// Mask passed to
[shuffle] and [shuffleMix].
On 2013/10/31 01:43:55, Cutch wrote:
> On 2013/10/30 17:39:27, zra wrote:
> > Is there any way to refactor this so we only have one big list of constants?
> 
> Not without introducing a class that holds the shuffle constants (which I
don't
> want to do). static const members are not inherited through extend or with a
> mixin— I tried that.

Ah. Bummer.

[Cutch, 2013-11-01 02:15:03]

Patch Set 4 adds TODO and updates co19 tests.

[srdjan, 2013-11-01 05:51:41]

LGTM with comments

https://codereview.chromium.org/51373004/diff/150001/runtime/lib/simd128.cc
File runtime/lib/simd128.cc (right):

https://codereview.chromium.org/51373004/diff/150001/runtime/lib/simd128.cc#n...
runtime/lib/simd128.cc:288: if (m < 0 || m > 255) {
Add parentheses. Please factor the code out into ThrowMaskRangeException or
similar.

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1785: // Not a constant.
This comment is redundant, I think.

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1792: // Not a smi.
Ditto

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1797: if (mask < 0 || mask > 255) {
Add parentheses.

https://codereview.chromium.org/51373004/diff/150001/sdk/lib/typed_data/dart2...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://codereview.chromium.org/51373004/diff/150001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1374: if (m < 0 || m > 255) {
indent is off

[Cutch, 2013-11-01 07:06:30]

https://codereview.chromium.org/51373004/diff/150001/runtime/lib/simd128.cc
File runtime/lib/simd128.cc (right):

https://codereview.chromium.org/51373004/diff/150001/runtime/lib/simd128.cc#n...
runtime/lib/simd128.cc:288: if (m < 0 || m > 255) {
On 2013/11/01 05:51:41, srdjan wrote:
> Add parentheses. Please factor the code out into ThrowMaskRangeException or
> similar.

Done.

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1785: // Not a constant.
On 2013/11/01 05:51:41, srdjan wrote:
> This comment is redundant, I think.

Done.

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1792: // Not a smi.
On 2013/11/01 05:51:41, srdjan wrote:
> Ditto

Done.

https://codereview.chromium.org/51373004/diff/150001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:1797: if (mask < 0 || mask > 255) {
On 2013/11/01 05:51:41, srdjan wrote:
> Add parentheses.

Done.

https://codereview.chromium.org/51373004/diff/150001/sdk/lib/typed_data/dart2...
File sdk/lib/typed_data/dart2js/typed_data_dart2js.dart (right):

https://codereview.chromium.org/51373004/diff/150001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1374: if (m < 0 || m > 255) {
On 2013/11/01 05:51:41, srdjan wrote:
> indent is off

Done. Also added parenthesis. Here and elsewhere.

[Cutch, 2013-11-01 07:10:09]

Committed patchset #7 manually as r29723 (presubmit successful).