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

Switch VM to use new encoding of invocations in kernel AST #45340

Closed
alexmarkov opened this issue Mar 16, 2021 · 5 comments
Closed

Switch VM to use new encoding of invocations in kernel AST #45340

alexmarkov opened this issue Mar 16, 2021 · 5 comments
Assignees
@alexmarkov
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.

Comments

@alexmarkov
Copy link
Contributor

CFE introduced a new encoding of invocations in https://dart-review.googlesource.com/c/sdk/+/175480.
We should support new nodes in the VM and flip supportsNewMethodInvocationEncoding in VmTarget.

@a-siva @askeksa-google @johnniwinther
@alexmarkov alexmarkov added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Mar 16, 2021
@a-siva
Copy link
Contributor

a-siva commented Mar 16, 2021

Is this something we want to get done for the March milestone ?

@alexmarkov
Copy link
Contributor Author

There's no rush unless @johnniwinther would like to remove old invocation nodes soon.

@johnniwinther
Copy link
Member

There is no rush.

@askeksa-google
Copy link

Porting everything at once is a big lump of code; we should consider how we can do this somewhat incrementally.

We can at least split it up like this:

  1. Add support in the VM kernel parser and in all transformations used in JIT. Flip the flag for JIT.
  2. Add support in all AOT-specific transformations. Flip the flag for AOT.
  3. Remove support for the old encoding in transformations and VM (can be subdivided arbitrarily).
  4. Gradually take advantage of the extra (and more explicit) information in the new encoding to simplify the VM code and improve optimizations.

It would be good if we could subdivide the first two points more, especially since flipping the flag will affect all kernel transformations, not just the ones in the SDK. Some options could be:

  • Add a separate testing mode for the new encoding, so we can incrementally work on supporting that while running the old one.
  • Make a kernel transformer from the new to the old encoding and move that gradually through the pipeline as we port more and more transformations.

As for step 4 above, we should consider redesigning some of the flow graph nodes to match the new encoding better. Many of the distinctions that are implicit in the old encoding and explicit in the new one (dynamic vs. non-dynamic calls, getter calls vs. tearoffs, implicit null check on == etc.) are currently deeply implicit in the VM. To take full advantage of the new encoding, we should make these explicit in the VM's internal structures as well.

@alexmarkov
Copy link
Contributor Author

Currently we cannot set front-end Target flags depending on the JIT/AOT mode, because we support 2-step kernel generation, where the first step is the front-end generating a kernel which could be used both in JIT and AOT. So distinguishing JIT from AOT and wiring this up is rather complex. It's much easier to do 1 and 2 in one step. Supporting incremental migration with a separate transformation from new to old encoding also looks like a lot of unnecessary extra work.

I think we should support new invocation encoding both in the VM kernel binary reader/flow graph builder and VM-specific kernel AST transformations (both AOT and modular) altogether and flip the flag in one go. That doesn't look too hard to me, and I would be happy to do this after I'm finished with my current tasks. We might need to enable/disable the flag multiple times if there are any problems with new encoding, so we'll keep support for old encoding for a while. Then, as a separate step we can cleanup support for the old encoding.

@alexmarkov alexmarkov self-assigned this Apr 28, 2021
dart-bot pushed a commit that referenced this issue May 4, 2021
@alexmarkov
[vm] Handle new AST invocation nodes in VM kernel transformations
df80241 
This change extends VM-specific kernel transformations to handle
new AST invocation nodes. The transformations may still generate
old nodes, but they should accept and handle new nodes coming from
the front-end.

TEST=Manual testing with new invocation nodes enabled.

Issue: #45340
Change-Id: I2de9f0eb00fcf844ba62fdc93b15a907c2d6b69d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197443
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue May 5, 2021
@alexmarkov
[vm] Support new kernel AST invocation nodes in the VM
b00233f 
TEST=Manual testing with new invocation nodes enabled.

Issue: #45340
Change-Id: I7eb5f03b9d9ac16c911812a5dbcd92ad43220278
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197585
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue May 5, 2021
@alexmarkov
[cfe] Use more precise types for FunctionInvocation and LocalFunction…
df3bb63 
…Invocation in type checker

Issue: #45340
Change-Id: I963b3a3b1143a9fbd0eac700daa11488aa5b183a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198182
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue May 7, 2021
@alexmarkov
[vm/ffi] Refactor a few duplicated AST patterns in FFI transformer
7eb2805 
This change is a pure refactoring. It extracts 3 duplicated code
snippets into helper methods. This is needed to reduce number of
places where PropertyGet and MethodInvocation nodes are created, as
these nodes are going to be replaced with InstanceGet and
InstanceInvocation nodes soon.

Issue: #45340
Change-Id: I694805a3761fd389ac8ee005d12ffb9bb9543ea7
TEST=ci
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198581
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue May 9, 2021
@alexmarkov
[cfe] Improve support for new invocation nodes in constant evaluator
237c6ee 
* FunctionInvocation and LocalFunctionInvocation nodes are now supported.
* Handling of List getters is added to InstanceGet.
* Removed incorrect constant evaluation of .hashCode and .runtimeType
  for List constants.

Issue: #45340
Change-Id: I73f3e64e805c0753325463b3e777a6c1b7d0b5fb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198181
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue May 9, 2021
@alexmarkov
[vm] Enable new kernel AST invocation nodes for the VM
53e3c07 
TEST=ci

Issue: #45340
Change-Id: Ibb307d8df8ff4ae8f2efad177880634ec9e27905
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197586
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
dart-bot pushed a commit that referenced this issue May 10, 2021
@mkustermann
Revert "[vm] Enable new kernel AST invocation nodes for the VM"
3136433 
This reverts commit 53e3c07.

Reason for revert: This change has seemingly caused errors of the form
```
stderr >> Unhandled exception:
stderr >> Invalid argument(s): Missing canonical name for Reference to <XXX>
stderr >> #0      BinaryPrinter.writeNullAllowedReference (package:kernel/binary/ast_to_binary.dart:894:9)
stderr >> #1      BinaryPrinter.visitTypedefType (package:kernel/binary/ast_to_binary.dart:2410:5)
stderr >> #2      TypedefType.accept (package:kernel/ast.dart:11115:42)
stderr >> #3      BinaryPrinter.writeNode (package:kernel/binary/ast_to_binary.dart:403:10)
stderr >> #4      BinaryPrinter.writeOptionalNode (package:kernel/binary/ast_to_binary.dart:496:7)
```

Issue #45966

Original change's description:
> [vm] Enable new kernel AST invocation nodes for the VM
>
> TEST=ci
>
> Issue: #45340
> Change-Id: Ibb307d8df8ff4ae8f2efad177880634ec9e27905
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197586
> Commit-Queue: Alexander Markov <alexmarkov@google.com>
> Reviewed-by: Johnni Winther <johnniwinther@google.com>
> Reviewed-by: Aske Simon Christensen <askesc@google.com>

TBR=vegorov@google.com,alexmarkov@google.com,johnniwinther@google.com,askesc@google.com

Change-Id: I05d010378cfcc5ba40f2be58b01b3ed27a8fc31e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Issue: #45340
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199000
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue May 14, 2021
@alexmarkov
[vm] Do not rely on function access kind in FunctionInvocation nodes
56c3d1e 
New FunctionInvocation nodes have explicit static type information such
as function access kind and function type. However, an attempt to use
this information revealed that it is less accurate compared to the old
call site attributes metadata
(see #46003 for details).

This change reverts back to using call site metadata when building
flow graph for FunctionInvocation nodes. It fixes regression of
ListCopy benchmark with new invocation nodes.

TEST=benchmarks/ListCopy/dart/ListCopy.dart with new invocation nodes

Issue: #45340
Issue: #46003
Change-Id: I73e5fae49b8056365211989e6e656544c79bcc50
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199563
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue May 20, 2021
@alexmarkov
[cfe] Fix types in spreads with new invocation nodes
2b3713b 
Fixes #46076
Issue #45340

Change-Id: If6f83d4e74944f1220f8372a0aa73857a6289596
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/200704
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue May 20, 2021
@alexmarkov
[vm] Enable new kernel AST invocation nodes for the VM, take 2
88021fd 
This change was already reviewed at
https://dart-review.googlesource.com/c/sdk/+/197586.

Bugfix is in the separate change:
https://dart-review.googlesource.com/c/sdk/+/199365.

TEST=ci

Issue: #45340
Change-Id: I4311909e4893af53e88895512f03d3ef84c6bc5f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199366
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue May 26, 2021
@alexmarkov
[vm] Generate new invocation node is FFI transformers
7e76ac6 
This change replaces generation of old invocation AST nodes
(such as PropertyGet, PropertySet and MethodInvocation) in FFI
transformers with new nodes (InstanceGet, InstanceSet,
InstanceInvocation). The old nodes will be deleted eventually.

TEST=existing tests

Issue: #45340
Change-Id: I7c01cc23c257514b4c89295a31ce63c947c18e23
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201222
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Jun 9, 2021
@alexmarkov
[vm/kernel] Generate new invocation nodes in async transformers
6e989df 
This change replaces generation of old invocation AST nodes
(such as PropertyGet, PropertySet and MethodInvocation) in async
transformers with new nodes (InstanceGet, InstanceSet,
InstanceInvocation, LocalFunctionInvocation).
The old nodes will be deleted eventually.

TEST=existing tests

Issue: #45340
Change-Id: I8c2ead9509cfd2def2f75f9d82dc13b2a9490fdf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202801
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
dart-bot pushed a commit that referenced this issue Jun 14, 2021
@alexmarkov
[vm] Generate new invocation node in the VM-specific transformations
dd1f384 
This change updates the remaining places in the VM-specific
transformations to generate new invocation nodes (InstanceInvocation,
EqualsNull, LocalFunctionInvocation) instead of old invocation
nodes such as MethodInvocation.
The old nodes will be deleted eventually.

TEST=existing tests

Issue: #45340
Change-Id: I00a845e8191f79584c250f57214dd5fb4d6241ed
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203443
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue Jun 16, 2021
@alexmarkov
[vm/kernel] Generate new invocation nodes in ffi-native transformation
e3d51d0 
This change replaces generation of old invocation AST nodes
(MethodInvocation) with new nodes (FunctionInvocation) in the recently
added ffi native transformation.
The old nodes will be deleted eventually.

Follow-up to https://dart-review.googlesource.com/c/sdk/+/170092.

TEST=ci

Issue: #45340
Change-Id: Ifbf882fbc96bb1307fbe2aac334b7427276146c8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203740
Reviewed-by: Clement Skau <cskau@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue Jun 16, 2021
@alexmarkov
[vm/kernel] Revise call site metadata for new invocation nodes
93c812e 
Call site attributes metadata is used to pass receiver static type
from kernel AST to the VM. In certain cases, call site metadata is
superseded by the new invocation nodes. This change revises how call
site attributes metadata is generated in call site annotator and
used in the VM for the new invocation nodes.

* When building flow graph for FunctionInvocation,
  the decision to generate unchecked calls is now done by
  looking only at the function access kind, without taking static
  receiver type into account. Call site annotator now verifies that
  static receiver type matches function access kind for
  FunctionInvocation nodes to make sure nothing is lost during this
  transition. Also, flow graph builder asserts that
  function access kind is either Function or FunctionType.

* Call site metadata is no longer generated for LocalFunctionInvocation
  and FunctionInvocation nodes, as it is no longer used by the VM.

* Call site annotator now verifies that InstanceInvocation nodes
  cannot be used for unchecked closure calls. Flow graph builder no
  longer recognizes unchecked closure calls for InstanceInvocation
  and DynamicInvocation nodes.

* When generating flow graph for DynamicInvocation and DynamicSet,
  call site metadata is no longer taken into account (metadata is not
  generated by call site annotator for these nodes).
  Also, 'this' receiver is not recognized for DynamicInvocation and
  DynamicSet nodes as it may potentially violate dynamic call semantics.

TEST=ci

Issue: #45340
Change-Id: Ia8c5e547965ac8d7a17908d4be4bd048e5cfb23f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203668
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alexmarkov alexmarkov

Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@a-siva @johnniwinther @askeksa-google @alexmarkov