[L] Change in dart/sdk[main]: [dart2wasm] Simd for vector math
Kevin Moore (Gerrit)
Kevin Moore voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Moore
| Commit-Queue | +1 |
1 comment
The world of SIMD vector optimization is CRAZY!
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore added 2 comments![]( "Open in Gerrit")
WasmF64x2 get _zeroF64x2 => WasmF64x2.splat(WasmF64.fromDouble(0.0));There should be SOME way to make this a const, right?
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 11 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
11 comments
lgtm with comments
if (lanesExp is ConstantExpression &&As more instructions accept lane arguments, I think it makes sense to introduce a helper method here: `List<int> _extractLaneIndices()`
(similar to our existing `_extractMemoryOperands`, `extractIntValue`, etc)
external bool get allTrue;Should we have this for all shapes?
external factory WasmF64x2.fromDoubles(double lane0, double lane1);Could we rename this to e.g. `WasmF64x2.fromLaneValues()` (or some better name) and make the arguments `WasmF64`?
Then we can add also `WasmF32x4.fromLaneValues()`, `WasmI8x16.fromLanValues()` etc
external WasmF64x2 pmin(WasmF64x2 other);Could you add it to WasmF32x4 as well?
external WasmF64x2 shuffle(List<int> lanes);It seems the actual shuffle is an operation on 3 operands: `(v128, v128, i8[16]) -> v128`. So if we expose the shuffle instruction, why not expose it with full power?
If this is only about permuting the contents of one v128 value, then why not expose `swizzle` instead?
// Expect.equals(vPmin.extractLane(0).toDouble(), 10.5);What is this commented code here?
(Did antigravity write it, tests were not passing and the solution was to comment out the code to make tests pass 😄)
// vPmax = v1.pmax(vNaN); // pmax(10.5, NaN) -> 10.5The difference between min/max and pmin/pmax are handling of NaNs, so it seems relevant to test those.
other.runtimeType == runtimeType &&I don't think we should compare `runtimeType`.
.abs()Why the `.abs()` calls here? Shouldn't they be all non-negative?
WasmF64x2 get _zeroF64x2 => WasmF64x2.splat(WasmF64.fromDouble(0.0));There should be SOME way to make this a const, right?
Probably
You can add a const constructor:
```
class WasmV128 {
@pragma('wasm:entry-point')
final List<Object> laneValues;
const WasmV128.literal(this.laneValues);
}
extension type const WasmF64x2(WasmV128 value) {
const WasmF64x2.literal(WasmF64 a, WasmF64 b) : this(WasmV128.literal([a, b]));
}
```
and add corresponding backend support (you can have a look at how we do it for `WasmI32.literal` and `WasmArray.literal`)Then you can probably use `const WasmF64x2.literal(...)`
Related details
- Kevin Moore
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 3 comments![]( "Open in Gerrit")
Forgot to send these last week.
other.runtimeType == runtimeType &&It's generally not great to compare `runtimeType`. If the subtypes shouldn't be equal to each other they should override `==` themselves. `OffsetBase` shouldn't have an `==`.
final class Offset extends OffsetBase {Not that it matters for a test but it seems like maybe these could even be extension types on `WasmF64x2` so you don't even need the wrapper object. The static type system can make sure you don't mess up `Offset` vs. `Size` but there's no reason for the runtime type system to worry about it.
I guess unless you really need the `==` and `toString`. I would say in this case just letting `==` fall through to `WasmV128.==` works.
Related details
- Kevin Moore
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore voted and added 11 comments![]( "Open in Gerrit")
Votes added by Kevin Moore
| Commit-Queue | +1 |
11 comments
As more instructions accept lane arguments, I think it makes sense to introduce a helper method here: `List<int> _extractLaneIndices()`
(similar to our existing `_extractMemoryOperands`, `extractIntValue`, etc)
Done
Should we have this for all shapes?
They are available for all integer types. Will follow-up with the rest
external factory WasmF64x2.fromDoubles(double lane0, double lane1);Could we rename this to e.g. `WasmF64x2.fromLaneValues()` (or some better name) and make the arguments `WasmF64`?
Then we can add also `WasmF32x4.fromLaneValues()`, `WasmI8x16.fromLanValues()` etc
Done
Could you add it to WasmF32x4 as well?
I'm focusing on WasmF64x2 – will follow-up
It seems the actual shuffle is an operation on 3 operands: `(v128, v128, i8[16]) -> v128`. So if we expose the shuffle instruction, why not expose it with full power?
If this is only about permuting the contents of one v128 value, then why not expose `swizzle` instead?
Done
// Expect.equals(vPmin.extractLane(0).toDouble(), 10.5);What is this commented code here?
(Did antigravity write it, tests were not passing and the solution was to comment out the code to make tests pass 😄)
Done
// vPmax = v1.pmax(vNaN); // pmax(10.5, NaN) -> 10.5The difference between min/max and pmin/pmax are handling of NaNs, so it seems relevant to test those.
Done
I don't think we should compare `runtimeType`.
Done
Why the `.abs()` calls here? Shouldn't they be all non-negative?
Done
WasmF64x2 get _zeroF64x2 => WasmF64x2.splat(WasmF64.fromDouble(0.0));Martin KustermannThere should be SOME way to make this a const, right?
Probably
You can add a const constructor:
```
class WasmV128 {
@pragma('wasm:entry-point')
final List<Object> laneValues;
const WasmV128.literal(this.laneValues);
}extension type const WasmF64x2(WasmV128 value) {
const WasmF64x2.literal(WasmF64 a, WasmF64 b) : this(WasmV128.literal([a, b]));
}
```
and add corresponding backend support (you can have a look at how we do it for `WasmI32.literal` and `WasmArray.literal`)Then you can probably use `const WasmF64x2.literal(...)`
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore added 2 comments![]( "Open in Gerrit")
other.runtimeType == runtimeType &&Kevin MooreIt's generally not great to compare `runtimeType`. If the subtypes shouldn't be equal to each other they should override `==` themselves. `OffsetBase` shouldn't have an `==`.
Done
Not that it matters for a test but it seems like maybe these could even be extension types on `WasmF64x2` so you don't even need the wrapper object. The static type system can make sure you don't mess up `Offset` vs. `Size` but there's no reason for the runtime type system to worry about it.
I guess unless you really need the `==` and `toString`. I would say in this case just letting `==` fall through to `WasmV128.==` works.
I want to stay kinda close to the Flutter behavior, so I'm leaving this here for now.
I LOVE the extension type of WasmF64x2 – but there is no path to making that a real change to Flutter.
Related details
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Bosch (Gerrit)
Nate Bosch added 1 comment![]( "Open in Gerrit")
Can you add details about how the code is changing to the commit message?
go/cl-descriptions
Related details
- Kevin Moore
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |