Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[dart2wasm] Implement structs backed by TypedData #55083

Closed
dcharkes opened this issue Mar 2, 2024 · 3 comments
Closed

[dart2wasm] Implement structs backed by TypedData #55083

dcharkes opened this issue Mar 2, 2024 · 3 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. area-dart2wasm Issues for the dart2wasm compiler. library-ffi

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Mar 2, 2024 •
edited
Loading

Currently, dart2wasm assumes that all structs are backed by a Pointer.

sdk/pkg/dart2wasm/lib/intrinsics.dart

Lines 186 to 193 in 42ccd79

// _Compound._typedDataBase
if (cls == translator.ffiCompoundClass && name == '_typedDataBase') {
// A compound (subclass of Struct or Union) is represented by its i32
// address. The _typedDataBase field contains a Pointer pointing to the
// compound, whose representation is the same.
codeGen.wrap(receiver, w.NumType.i32);
return w.NumType.i32;
}

dart2wasm can currently assume this, because it does not implement support for:

  • Returning struct by value from FFI calls.
  • Struct.create.

If either of these features is implemented in dart2wasm, compounds cannot be represented as the int address of the pointer.

Also, it would break nested compounds if the suggestion in #54892 is implemented, as the _offsetInBytes would not be saved anywhere.
@dcharkes dcharkes added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi area-dart2wasm Issues for the dart2wasm compiler. labels Mar 2, 2024
@dcharkes dcharkes changed the title [dart2wasm] Structs backed by TypedData [dart2wasm] Implement structs backed by TypedData Mar 2, 2024
@dcharkes
Copy link
Contributor Author

dcharkes commented Mar 4, 2024 •
edited
Loading

Also, it would break nested compounds if the suggestion in #54892 is implemented, as the _offsetInBytes would not be saved anywhere.

@mkustermann @osa1 @eyebrowsoffire Are you aware of any uses of nested structs or arrays in current dart:ffi usage (only skwasm right?) in dart2wasm?

CL breaking nested compounds in dart2wasm: https://dart-review.googlesource.com/c/sdk/+/354226 (We have no dart2wasm tests exercising nested compounds. So nothing goes red.)

@mkustermann
Copy link
Member

We currently don't consider dart:ffi as a supported thing that end users will use. There's a limited use in flutter web engine, which is supported. I believe that limited use doesn't use structs by value and is also restricted to static @Native interop usages. None of the ffi tests in the tests/ffi test suite run on wasm - we only have tests/web/wasm/ffi_native_test.dart.

Longer term we'll require multiple memories for wasm. We'll have to see what functionality of dart:ffi we want to support and how to expose it to users - maybe through dart:ffi library - maybe not.

So for now we should ensure that the ffi features which the flutter web engine requires are tested in tests/web/wasm/ffi_native_test.dart and we keep that supported.

copybara-service bot pushed a commit that referenced this issue Mar 26, 2024
@dcharkes
[vm/ffi] Add _Compound _offsetInBytes field
0bcb0c9 
This CL changes compounds (structs, unions, and arrays) to be backed
by a TypedDataBase and an int offset.

Before this CL, the compounds where only backed by a TypedDataBase.
This leads to the following issues:

1. Access to nested structs required code for allocating new typed data
   views or pointers, which the optimizer then had to prevent from
   being allocated after inlining.
2. Runtime branching on whether the TypedDataBase was a Pointer or
   TypedData increased code size and prevented inlining.
   #54892
   This could not be properly optimized if in AOT both typed
   data and pointer were flowing into the same compound.
3. Constructing TypedData views required calculating the length of the
   view.

After this CL, accessing nested compounds will lead to accesses on the
original TypedDataBase with an extra offset.

This removes the polymorphism on TypedData vs Pointer, because the
final int/float/Pointer accesses in nested compounds operate on
TypedDataBase.

Also, it simplifies creating an `offsetBy` accessor, because it will
no longer have to be polymorphic in typed data vs pointer, nor will it
have to calculate the length of the field.

Implementation details:

* The changes in the CFE and patch files are straightforward.
* VM: Struct-by-value returns (or callback params) are initialized
  with an offsetInBytes of 0.
* VM: Struct-by-value arguments (and callback return) need to read out
  the offsetInBytes. Before this CL we were passing in the TypedData
  as tagged value. With this CL we are passing the TypedData as tagged
  value and the offset as unboxed int, from 1 IL input to 2 IL
  inputs. (The alternative would have been to take the compound as
  a tagged value, but that would have prevented optimizations from not
  allocating the compound object in the optimizer.
  The FfiCallInstr is updated to have two definitions for the case
  where we were passing in the TypedData previously.
  The NativeReturnInstr is refactored to be able to take two inputs
  instead of 1. (Note that we don't have VariadicInstr only
  VariadicDefinition in the code base. So the instruction is _not_
  implemented as variadic, rather as having a fixed length of 2.)
* dart2wasm does no longer support nested compounds due to the
  compound implementation only storing a pointer address.
  #55083

Intending to land this after
https://dart-review.googlesource.com/c/sdk/+/353101.

TEST=test/ffi

CoreLibraryReviewExempt: VM and WASM-only implementation change.
Closes: #54892
Bug: #44589
Change-Id: I8749e21094bf8fa2d5ff1e48b6b002c375232eb5
Cq-Include-Trybots: dart-internal/g3.dart-internal.try:g3-cbuild-try
Cq-Include-Trybots: dart/try:vm-aot-android-release-arm64c-try,vm-aot-android-release-arm_x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-aot-obfuscate-linux-release-x64-try,vm-aot-optimization-level-linux-release-x64-try,vm-aot-win-debug-arm64-try,vm-aot-win-debug-x64-try,vm-aot-win-debug-x64c-try,vm-appjit-linux-debug-x64-try,vm-asan-linux-release-x64-try,vm-checked-mac-release-arm64-try,vm-eager-optimization-linux-release-ia32-try,vm-eager-optimization-linux-release-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-fuchsia-release-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-reload-rollback-linux-debug-x64-try,vm-ubsan-linux-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try,vm-win-release-ia32-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354226
Reviewed-by: Tess Strickland <sstrickl@google.com>
@mkustermann
Copy link
Member

I'm going to close this in favor of the more general Implement dart:ffi for the web.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. area-dart2wasm Issues for the dart2wasm compiler. library-ffi
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dcharkes @mkustermann