Don't use a float32list as storage for simd.

Don't use a float32list as storage for simd.

R=lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38153
Reverted: https://code.google.com/p/dart/source/detail?r=38157

Committed: https://code.google.com/p/dart/source/detail?r=38194
Reverted: https://code.google.com/p/dart/source/detail?r=38231

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

[Cutch, 2014-07-09 20:35:10]

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

https://codereview.chromium.org/278593003/diff/20001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1464: var view = new
Uint32List.view(_list.buffer);
This view can be made static and shared.

https://codereview.chromium.org/278593003/diff/20001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2432: var view = new
Uint32List.view(_list.buffer);
Should this list view also be static?

[floitsch, 2014-07-09 23:00:08]

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

https://codereview.chromium.org/278593003/diff/20001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1464: var view = new
Uint32List.view(_list.buffer);
On 2014/07/09 20:35:10, Cutch wrote:
> This view can be made static and shared.

Done.

https://codereview.chromium.org/278593003/diff/20001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2432: var view = new
Uint32List.view(_list.buffer);
On 2014/07/09 20:35:10, Cutch wrote:
> Should this list view also be static?

Done.

[Lasse Reichstein Nielsen, 2014-07-10 09:34:12]

LGTM with comments.

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

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1267: /// All arguments must
be non-null doubles.
Are they doubles or do they need to be doubles? From the code, I assume they
*are* doubles.
Doubles are always non-null.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1274: /// All arguments must
be truncated.
Are they truncated or do they need to be truncated? "Must be" can mean both.
From the code I assume that "All arguments are already Float32 values".

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1281:
Float32x4.fromInt32x4Bits(Int32x4 x)
Could this just be a factory method, instead of using an extra helper function
and an extra constructor? E.g.:
  factory Float32x4.fromInt32x4bits(int32x4 x) { 
    _uint32view[0] = x.x;
    _uint32view[1] = x.y;
    _uint32view[2] = x.z;
    _uint32view[3] = x.w;
    return new Float32x4._truncated(_list[0], _list[1], _list[2], _list[3]);
 }

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1353: return new
Int32x4._truncated(_cx == true ? -1 : 0,
Why the '== true'? The "x" is known to be double, so its '<' will return a
boolean.

If you really don't trust a value to be bool, either use identical or 'true ==
x', since a non-bool value can override == as well.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1460: return new
Float32x4._truncated(_x, _y, _z, _w);
This trusts that lowerLimit.x really is a Float32. I guess that's ok - if
someone makes a Float32x4 implementation that has invalid values, it's ok to
give wrong results.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1471: mx = (view[0] &
0x80000000) >> 31;
This is dart2js, so >> treats the operand as *uint32* - so this should be
correct. And it's correct in real Dart as well. Still might be worth a comment.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1472: my = (view[1] &
0x80000000) >> 31;
How about only shifting this by 30, instad of shifting it back up by one later.
Ditto for the next two.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1881:
Int32x4.fromFloat32x4Bits(Float32x4 f)
Make this a single factory constructor?

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2287: /// Extracted x value.
Returns false for 0, true for any other value.
code-quote `false` and `true`?

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2322: /// Select bit from
[falseValue] when bit in [this] is off.
So this method merges the *bits* of float32s - that sounds .... useful? The
values of this Int32x4 should all be either 0 or -1 for this to be a clean
merge. Documented anywhere?

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2325: var intView = new
Int32List.view(floatList.buffer);
Why not use Float32x4._uint32view? I see no reason for this to be signed.

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/flo...
File tests/lib/typed_data/float32x4_test.dart (right):

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/flo...
tests/lib/typed_data/float32x4_test.dart:444: testBadArguments();
Please test the truncation on all of the following double values (both positive
and negative):

0.0
5e-324
2.225073858507201e-308
2.2250738585072014e-308
0.9999999999999999
1.0
1.0000000000000002
4294967295.0
4294967296.0
4503599627370495.5
9007199254740992.0
1.7976931348623157e+308
0.49999999999999994
4503599627370497.0
9007199254740991.0
Infinity
NaN

and the following float32 values (positive and negative) which should not be
modified:

1.401298464324817e-45
1.1754942106924411e-38
1.1754943508222875e-38
0.9999999403953552
1.0000001192092896
8388607.5
8388608.0
3.4028234663852886e+38
8388609.0
16777215.0

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/int...
File tests/lib/typed_data/int32x4_test.dart (right):

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/int...
tests/lib/typed_data/int32x4_test.dart:47: Expect.equals(-1537836912
,int32x4.x);
Comma position? Ditto below.

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/int...
tests/lib/typed_data/int32x4_test.dart:62: Expect.equals(-1537836912
,int32x4.w);
How about some edge cases? Like:
  0x80000000
  0x7fffffff
both plus and minus.

[floitsch, 2014-07-10 16:41:59]

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

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1267: /// All arguments must
be non-null doubles.
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Are they doubles or do they need to be doubles? From the code, I assume they
> *are* doubles.
> Doubles are always non-null.

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1274: /// All arguments must
be truncated.
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Are they truncated or do they need to be truncated? "Must be" can mean both.
> From the code I assume that "All arguments are already Float32 values".

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1281:
Float32x4.fromInt32x4Bits(Int32x4 x)
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Could this just be a factory method, instead of using an extra helper function
> and an extra constructor? E.g.:
>   factory Float32x4.fromInt32x4bits(int32x4 x) { 
>     _uint32view[0] = x.x;
>     _uint32view[1] = x.y;
>     _uint32view[2] = x.z;
>     _uint32view[3] = x.w;
>     return new Float32x4._truncated(_list[0], _list[1], _list[2], _list[3]);
>  }

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1353: return new
Int32x4._truncated(_cx == true ? -1 : 0,
On 2014/07/10 09:34:11, Lasse Reichstein Nielsen wrote:
> Why the '== true'? The "x" is known to be double, so its '<' will return a
> boolean.
> 
> If you really don't trust a value to be bool, either use identical or 'true ==
> x', since a non-bool value can override == as well.

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1460: return new
Float32x4._truncated(_x, _y, _z, _w);
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> This trusts that lowerLimit.x really is a Float32. I guess that's ok - if
> someone makes a Float32x4 implementation that has invalid values, it's ok to
> give wrong results.

Acknowledged.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1471: mx = (view[0] &
0x80000000) >> 31;
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> This is dart2js, so >> treats the operand as *uint32* - so this should be
> correct. And it's correct in real Dart as well. Still might be worth a
comment.

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1472: my = (view[1] &
0x80000000) >> 31;
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> How about only shifting this by 30, instad of shifting it back up by one
later.
> Ditto for the next two.

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:1881:
Int32x4.fromFloat32x4Bits(Float32x4 f)
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Make this a single factory constructor?

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2287: /// Extracted x value.
Returns false for 0, true for any other value.
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> code-quote `false` and `true`?

Done.

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2322: /// Select bit from
[falseValue] when bit in [this] is off.
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> So this method merges the *bits* of float32s - that sounds .... useful? The
> values of this Int32x4 should all be either 0 or -1 for this to be a clean
> merge. Documented anywhere?

The function works as advertised. Clearly 0 or -1 are the most natural choices,
but I can imagine other uses...

https://codereview.chromium.org/278593003/diff/60001/sdk/lib/typed_data/dart2...
sdk/lib/typed_data/dart2js/typed_data_dart2js.dart:2325: var intView = new
Int32List.view(floatList.buffer);
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Why not use Float32x4._uint32view? I see no reason for this to be signed.

Done.

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/flo...
File tests/lib/typed_data/float32x4_test.dart (right):

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/flo...
tests/lib/typed_data/float32x4_test.dart:444: testBadArguments();
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Please test the truncation on all of the following double values (both
positive
> and negative):
> 
> 0.0
> 5e-324
> 2.225073858507201e-308
> 2.2250738585072014e-308
> 0.9999999999999999
> 1.0
> 1.0000000000000002
> 4294967295.0
> 4294967296.0
> 4503599627370495.5
> 9007199254740992.0
> 1.7976931348623157e+308
> 0.49999999999999994
> 4503599627370497.0
> 9007199254740991.0
> Infinity
> NaN
> 
> and the following float32 values (positive and negative) which should not be
> modified:
> 
> 1.401298464324817e-45
> 1.1754942106924411e-38
> 1.1754943508222875e-38
> 0.9999999403953552
> 1.0000001192092896
> 8388607.5
> 8388608.0
> 3.4028234663852886e+38
> 8388609.0
> 16777215.0

Done.

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/int...
File tests/lib/typed_data/int32x4_test.dart (right):

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/int...
tests/lib/typed_data/int32x4_test.dart:47: Expect.equals(-1537836912
,int32x4.x);
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> Comma position? Ditto below.

Done.

https://codereview.chromium.org/278593003/diff/60001/tests/lib/typed_data/int...
tests/lib/typed_data/int32x4_test.dart:62: Expect.equals(-1537836912
,int32x4.w);
On 2014/07/10 09:34:12, Lasse Reichstein Nielsen wrote:
> How about some edge cases? Like:
>   0x80000000
>   0x7fffffff
> both plus and minus.

Done.

[floitsch, 2014-07-10 20:25:30]

PTAL. Rebase was painful...

And I have one question.

https://codereview.chromium.org/278593003/diff/100002/sdk/lib/_internal/lib/n...
File sdk/lib/_internal/lib/native_typed_data.dart (right):

https://codereview.chromium.org/278593003/diff/100002/sdk/lib/_internal/lib/n...
sdk/lib/_internal/lib/native_typed_data.dart:1307: static final Uint32List
_uint32view = _list.buffer.asUint32List();
This is now a warning.
Can/should we change the return-type of NativeFloat32List.buffer?

[Lasse Reichstein Nielsen, 2014-07-11 11:59:12]

lgtm

https://codereview.chromium.org/278593003/diff/100002/sdk/lib/_internal/lib/n...
File sdk/lib/_internal/lib/native_typed_data.dart (right):

https://codereview.chromium.org/278593003/diff/100002/sdk/lib/_internal/lib/n...
sdk/lib/_internal/lib/native_typed_data.dart:1307: static final Uint32List
_uint32view = _list.buffer.asUint32List();
It shouldn't give a warning, if it does, something is wrong.
Which warning does it give?

[floitsch, 2014-07-11 12:02:36]

https://codereview.chromium.org/278593003/diff/100002/sdk/lib/_internal/lib/n...
File sdk/lib/_internal/lib/native_typed_data.dart (right):

https://codereview.chromium.org/278593003/diff/100002/sdk/lib/_internal/lib/n...
sdk/lib/_internal/lib/native_typed_data.dart:1307: static final Uint32List
_uint32view = _list.buffer.asUint32List();
On 2014/07/11 11:59:12, Lasse Reichstein Nielsen wrote:
> It shouldn't give a warning, if it does, something is wrong.
> Which warning does it give?

Never mind. It was using the old SDK for the checking. I need to update my
editor...

[floitsch, 2014-07-11 12:03:40]

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

[floitsch, 2014-07-11 13:27:27]

Still running the tests, but switching the storage of Int32x4Lists to Int32List
(instead of Uint32List) seems to fix things.

(broke pkg/typed_data/test/typed_buffers_test/)

I will add a test, too.

(Basically the Int32x4List returned an Int32x4 that wasn't correctly truncated).

[floitsch, 2014-07-11 15:54:14]

Added tests and fixed buffer.asInt32x4List.

[Lasse Reichstein Nielsen, 2014-07-11 23:56:27]

lgtm

[floitsch, 2014-07-14 15:02:45]

Committed patchset #10 manually as r38194 (presubmit successful).

[Bill Hesse, 2014-07-15 11:17:04]

The test lib/typed_data/int32x4_test is failing on Safari after this commit:
http://chromegw.corp.google.com/i/client.dart/builders/dart2js-safari-mac10.7...

[floitsch, 2014-07-15 14:56:54]

Updated status file.

[floitsch, 2014-07-15 14:58:27]

Committed patchset #12 manually as r38236 (presubmit successful).

[sra1, 2014-07-15 18:59:33]

What is the motivation for this change?

This change adds 4k to swarm.  Can it be done less expensively?