Skip to content

Create analogue to LoadNativeField for non-Object fields #42793

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.

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

Open
sstrickl opened this issue Jul 22, 2020 · 8 comments
Open

Create analogue to LoadNativeField for non-Object fields #42793

sstrickl opened this issue Jul 22, 2020 · 8 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@sstrickl
Copy link
Contributor

Currently, we can load <X>Ptr-typed fields in raw (e.g.,<Y>Layout) objects by using LoadNativeField. However, no analogue exists for fields that are not boxed types (e.g., uint32_t-typed fields).

This came up while working on #40813, where I needed to be able to inspect the number of fixed and optional parameters for a closure function where the closure function is not known statically, which requires extracting the value of the packed_fields_ field, which is of type uint32_t.

As a short-term workaround, I have added the right entries into the helper methods used by the LoadIndexed instruction to make the contents of a FunctionLayout object addressable as if the non-tagged portion was an external uint32_t array. A better solution for the long term would be to introduce another instruction (LoadUnboxedNativeField?) with appropriate Slot-like machinery for defining extractable unboxed fields from raw objects, and then remove the kFunctionCid hack in the various places it appears.

@sstrickl sstrickl added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Jul 22, 2020
@alexmarkov
Copy link
Contributor

@dcharkes We could also use such low-level load/store instructions in FFI implementation (e.g. in FlowGraphBuilder::UnwrapHandle()).

@sstrickl sstrickl self-assigned this Jul 24, 2020
@sstrickl
Copy link
Contributor Author

I've already got on a start on this on CL 155604, at least for the case I currently need (uint32_t), and it should be easily extendable to other unboxed field types. In that CL, LoadNativeField of an unboxed slot always returned a boxed version of the value, which might be nice to eventually handle unboxing appropriately in optimized code but I figure that's a later improvement.

@sstrickl
Copy link
Contributor Author

Just as a note, it's changed from always returning a boxed version to always returning an unboxed version, and allowing unboxed values on the expression stack in unoptimized code (still only when the unboxed values are added and consumed within a single basic block and not allowing calls while untagged values are on the stack).

Also, the first iteration of the backend changes only handles loads of kUnboxedUInt32 native fields (what I needed for this CL), so using it for the FFI implementation will need to both change the StoreField backend and extend the places in the LoadField backend that handles unboxed fields to handle Int64 (for archs where kUnboxedFfiIntPtr == kUnboxedInt64). (Am willing to do these changes since I already have the appropriate code paged in, but will do them as a separate CL.)

dart-bot pushed a commit that referenced this issue Aug 17, 2020
This CL performs the following checks in the invoke field dispatcher for
dynamic closure calls when lazy dispatchers are enabled:

* The provided function type arguments vector (if any) has the correct
  length.

* No function type arguments should be provided if the closure has
  delayed type arguments.

* All required arguments (positional in all modes, named in appropriate
  null safety modes) have been provided by the caller.

* If there are optional positional arguments, an appropriate number
  has been provided.

* If there are optional named arguments, their names are valid.

Since the runtime already handles checking the argument shapes when lazy
dispatchers are disabled, these checks are now completely removed from
closure bodies in all cases. Thus, the only remaining checks in closure
bodies are the type checks performed by AssertSubtype and
AssertAssignable when lazy dispatchers are enabled.

Changes in the Flutter Gallery:

* ARM7, release: -3.61% instructions, -2.19% total
* ARM7, sizeopt: -3.62% instructions, -2.55% total
* ARM8, release: -3.66% instructions, -1.98% total
* ARM8, sizeopt: -3.65% instructions, -2.37% total

Most of these changes are already exercised by existing tests such as
(but not limited to):

* corelib{,_2}/dynamic_nosuchmethod_test
* language{,_2}/call/call_test
* language{,_2}/closure/tearoff_dynamic_test
* language{,_2}/generic/function_bounds_test
* language{,_2}/parameter/named_with_conversions_test
* language{,_2}/vm/no_such_args_error_message_vm_test

I've added one test to specifically check the interaction between
dynamic calls and required named parameters. There is some coverage in
other NNBD tests, but those are not directly focused on testing this
specifically.

Other changes:

* Adds initial cached ranges for certain BinarySmiOp and ShiftIntegerOp
  instructions when the RHS is a constant, to avoid false negatives for
  deoptimization and throw checks prior to range analysis.

* Adds new slots for various Function fields.

* Adds the ability to define unboxed native slots, which are always
  unboxed after retrieval even in unoptimized code. In the first
  iteration, the backend only handles loads from Uint32 unboxed native
  slots. Part of #42793.

* Removed the special handling for loading from non-nullable int fields
  in AOT compilation. Instead, their treatment is unified with the
  treatment of the new unboxed native fields, since the source field is
  always unboxed and the result of the load is also always unboxed, as
  code involving them is always optimized.

Bug: #40813
Change-Id: Ia02aa3e872c1fefd906fd67b55021ea1797556e4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155604
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
@sstrickl
Copy link
Contributor Author

Just as a note, since the comments on here are now fairly out of date, Slots like those used in LoadNativeField/LoadFieldInstr have been adjusted to allow for native unboxed fields, and so LoadNativeField/LoadFieldInstr now works with these slots like any other.

I now have a CL that does the appropriate changes on the StoreInstanceFieldInstr side of things, and once that lands this can be closed.

@sstrickl
Copy link
Contributor Author

sstrickl commented May 3, 2021

Will leave this open even after the CL lands, because we should also see about migrating uses of LoadUntagged -> LoadField. (The CL mentioned in the last comment migrates all uses of StoreUntagged -> StoreInstanceField and removes StoreUntagged.)

dart-bot pushed a commit that referenced this issue May 3, 2021
Also creates LoadFromOffset/StoreToOffset variants across all
architectures (was missing in some) that take the register to
load to/store from, the memory address, and the size to load/store.

Fixes LoadFromOffset on X64 to use MOVSXD when the size is kFourBytes.

Creates slots for the data field of TypedDataBase/Pointer objects
and replaces the StoreUntagged instruction with uses of
StoreInstanceField instead to create uses of the new version of
the instruction.

BUG=#42793

TEST=Tests that involve FFI pointers or TypedData constructors.

Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-simarm64c-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-precomp-linux-product-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-kernel-precomp-linux-release-simarm64-try,vm-kernel-precomp-linux-release-simarm-try,vm-kernel-linux-debug-ia32-try,vm-kernel-linux-debug-x64-try,vm-kernel-linux-product-x64-try,vm-kernel-linux-release-simarm64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-x64-try,vm-kernel-linux-debug-x64c-try
Change-Id: I2c96e83bb086aa93c56b834e809e7141a32cfc35
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196924
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
@ds84182
Copy link
Contributor

ds84182 commented May 14, 2023

@dcharkes Some of the remaining usages of LoadUntagged in FFI is preventing the compiler from removing Pointer allocations.

import 'dart:ffi';

@pragma('vm:testing:print-flow-graph')
@pragma('vm:never-inline')
int bar(int l) => Pointer<Void>.fromAddress(l).address;
*** BEGIN CFG
After AllocateRegisters
==== file://<source>_::_bar (RegularFunction)
  0: B0[graph]:0 {
      v31 <- Constant(#TypeArguments: (H39d2e3b4) [Type: Never]) T{TypeArguments}
}
  2: B1[function entry]:2 {
      v2 <- Parameter(0) [-9223372036854775808, 9223372036854775807] T{int}
}
  3:     ParallelMove rdx <- C
  4:     v28 <- AllocateObject:24(cls=Pointer, v31 T{TypeArguments}) T{Pointer}
  5:     ParallelMove rax <- rax, rcx <- S+2
  6:     StoreField(v28 . PointerBase.data = v2 T{int} <int64>, NoStoreBarrier)
  8:     v18 <- LoadUntagged(v28 T{Pointer}, 8) T{*?}
 10:     v19 <- IntConverter(untagged->int64[tr], v18) T{int}
 11:     ParallelMove rax <- rcx
 12:     Return:14(v19 T{int})
*** END CFG

If I change some LoadUntagged instructions to LoadNativeField in kernel_to_il.cc the pointer allocation gets completely optimized out and the function becomes equivalent to int fn(int l) => l.

@dcharkes
Copy link
Contributor

We should be able to get rid of the pointer allocation indeed.

@ds84182 would you like to make a PR?

(Related, we'd like to completely unbox pointer with function arguments and returns, so that we can also avoid allocations there. That would likely work somewhat similar to unboxed int/double params/returns. #50777)

@ds84182
Copy link
Contributor

ds84182 commented May 15, 2023

I can try to put together a CL either today or tomorrow. May be easier to do on my workstation once I'm not OOO

ds84182 added a commit to ds84182/sdk that referenced this issue May 23, 2023
This allows the VM to elide FFI Pointer allocations during pointer
arithmetic.

Bug: dart-lang#42793
ds84182 added a commit to ds84182/sdk that referenced this issue May 26, 2023
This allows the VM to elide FFI Pointer allocations during pointer
arithmetic.

Bug: dart-lang#42793
ds84182 added a commit to ds84182/sdk that referenced this issue Jun 13, 2023
This allows the VM to elide FFI Pointer allocations during pointer
arithmetic.

Bug: dart-lang#42793
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

4 participants