[XL] Change in dart/sdk[main]: Share SIMD value operations via a dart._simd part
Modestas Valauskas (Gerrit)
Modestas Valauskas voted and added 4 comments![]( "Open in Gerrit")
Votes added by Modestas Valauskas
| Auto-Submit | +1 |
4 comments
PTAL. PS3 attempts to fix the remaining failures and address Stephen's feedback. It changed the approach from using mixins-from-another-library to using a shared library + part so we don't have to make private members public.
NativeFloat32x4List._externalStorage(this.storage);Modestas ValauskasHistorically it was important to keep this private.
Even nowadays, public names shared between private libraries can still leak to user code:
`(myF32x4List as dynamic).storage`.
Thank you, that's a very good point that I missed. I switched to parts in PS3.
Int32x4 withW(int w) {Modestas ValauskasThis was more efficient that the new code since it truncates only one element.
Done
final l = _i32Scratch;Modestas Valauskas`list` would be better - `l` is an abbreviation _and_ looks too much like `1`.
Done
Related details
- Kevin Moore
- Martin Kustermann
- Stephen Adams
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Modestas Valauskas (Gerrit)
Modestas Valauskas voted and added 1 comment![]( "Open in Gerrit")
Votes added by Modestas Valauskas
| Auto-Submit | +1 |
1 comment
PS5 regenerates wasm goldens and adds missing references to the new _simd library. PTAL
Related details
- Kevin Moore
- Martin Kustermann
- Stephen Adams
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Adams (Gerrit)
Stephen Adams added 3 comments![]( "Open in Gerrit")
I would like @kuste...@google.com and perhaps @veg...@google.com to comment before I revisit this issue.
part of dart._simd;/cc @johnni...@google.com
We have been moving away from this syntax to enforce that a part file can only be a part of one library, and usually in the same directory.
See https://github.com/dart-lang/language/issues/2358 for details.
A different approach: export shim code from the platform version that can be used by the common code. The shim would be a class with static methods, or an extension type to give some of those methods instance-method syntax. Something like this:
```
--- platform_simd.dart
extension Int32x4ListShim on Int32x4list {
Int32List get storage => _storage;
}extension Int32x4Shim on Int32x4 {
static Int32x4 newTruncated(int x, int y, int z, int w) =>
NativeInt32x4._truncated(x, y, z, w);
static int truncate(int x) => x.toSigned(32);
}
``````dart
--- simd_shared.dart
Int32x4 withX(int x) => NativeInt32x4._truncated(
NativeInt32x4._truncate(x),
y,
z,
w,
);
---->
Int32x4 withX(int x) => Int32x4Shim.newTruncated(
Int32x4Shim.truncate(x),
y,
z,
w
);
```
With the shims, the dependency between the common code and the platform version is explicit and contained, rather than the entire library's private namespace. For consistency, the platform code could use the same shim methods.
I still have misgivings about this whole approach. It is nice to have shared code, but only to the degree that the common code is easily maintained, and does not interfere with the evolution of the runtime. By coupling the runtimes together like this, it makes it harder to do something 'outside the pattern' in just one library.
I also worry is that at least one of the compilers will generate worse code because of the layer of abstraction or dispatch to mixin methods or failure to tree-shake unused declarations (either by open-world assumptions, or weaknesses in the algorithm).
Int32x4 operator |(Int32x4 other) => NativeInt32x4._truncated(This is incorrect for the _Dart_ bitwise operations on the web.
See https://dart.dev/resources/language/number-representation#bitwise-operations
The original code in the DDC ans dart2js runtimes has `JS` fragments to handle this. The `JS` fragments are not available on all platforms, and incompatible where they are available.
Related details
- Johnni Winther
- Martin Kustermann
- Modestas Valauskas
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Modestas Valauskas (Gerrit)
Modestas Valauskas added 3 comments![]( "Open in Gerrit")
Thank you Stephen for your feedback and I agree, let's wait for some other perspectives.
I do prefer the part feature for sharing code in this case:
- Publishing the private storage and constructors is only safe as long as we are careful.
- It introduces indirection in performance critical parts of our sdk (we can't guarantee that it will be inlined with a prefer inline pragma on all platforms, so the performance question applies here, too, I ran into one just yesterday https://dart-review.googlesource.com/c/sdk/+/523380).
- Since we have a feature for this issue, I think we should use it.
But I understand your concerns and I will not insist on us using parts when alternative approaches like yours are available and preferred. (Let's wait for a few other perspectives @l...@google.com I'd love to hear your thoughts here as well since https://github.com/dart-lang/language/issues/2358 is your issue)
Int32x4 operator |(Int32x4 other) => NativeInt32x4._truncated(This is incorrect for the _Dart_ bitwise operations on the web.
See https://dart.dev/resources/language/number-representation#bitwise-operationsThe original code in the DDC ans dart2js runtimes has `JS` fragments to handle this. The `JS` fragments are not available on all platforms, and incompatible where they are available.
I should probably give these a `SharedNaive...` prefix. My intent behind them is that the implementations that use these mixins can and should specialize the operations that they can implement more efficiently. I just checked again and the specializations do override these naive implementations on js and ddc side. (Or did you mean something else and I missed something?)
Related details
- Johnni Winther
- Lasse Nielsen
- Martin Kustermann
- Stephen Adams
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |