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

VM compiler should consistently use Cids/CallTargets instead of ICData #37575

Closed
rmacnak-google opened this issue Jul 18, 2019 · 4 comments
Closed

VM compiler should consistently use Cids/CallTargets instead of ICData #37575

rmacnak-google opened this issue Jul 18, 2019 · 4 comments
Assignees
@rmacnak-google
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P0 A serious issue requiring immediate resolution

Comments

@rmacnak-google
Copy link
Contributor

The VM compiler currently uses a mixture of Cids/CallTargets and ICData when it attempts to specialize calls. With switchable calls, the feedback in ICData is a subset of the feedback in Cids/CallTargets. This can lead to incorrect code if the compiler specializes for the subset and then inserts guards for the superset, leading to bugs like #37548
@rmacnak-google rmacnak-google added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Jul 18, 2019
@rmacnak-google rmacnak-google self-assigned this Jul 18, 2019
dart-bot pushed a commit that referenced this issue Jul 19, 2019
@rmacnak-google
[vm] When clearing ICData, also reset the megamorphic bit.
395e074 
After a reset due to reload, an ICData could end up with a single entry and have its megamorphic bit set with the MegamorphicCache having several entries. The compiler might specialize based on the ICData's single target and then insert guards based on the several classes in CallTargets. the

Bug: #37548
Bug: #37575
Change-Id: I3c22517ffc8c6936fcb90c6f11b2af3ba96cadfe
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109556
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
@rmacnak-google rmacnak-google mentioned this issue Aug 19, 2019
Closed
@mkustermann mkustermann added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Aug 21, 2019
@mkustermann
Copy link
Member

mkustermann commented Aug 21, 2019 •
edited
Loading

Making this P1, since there might be more places in the compiler where we consult the megamorphic cache multiple times and require the answer to be the same.

Ryan: If you don't have enough cycles, please let me know, then I can look into it as well.

@mkustermann mkustermann added P0 A serious issue requiring immediate resolution and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Aug 21, 2019
@mkustermann
Copy link
Member

(see also b/139789386)

dart-bot pushed a commit that referenced this issue Aug 22, 2019
@rmacnak-google
[vm, compiler] Always clone MegamorphicCaches before accessing on a b…
0176526 
…ackground compiler thread.

This is to prevent the compiler from becoming confused by mutations from the Dart execution thread.

Bug: #37575
Bug: #37595
Change-Id: I43c42feff54d76b780beb6e8d627f1b1641f4bee
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114054
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Sep 4, 2019
@rmacnak-google
[vm, compiler] Consistently use CallTargets instead of ICData or Mega…
67bb2b7 
…morphicCache.

Bug: #37575
Change-Id: I15f3862af380b04498cb58c8658aa6de76212733
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114445
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
@athomas
Copy link
Member

athomas commented Sep 10, 2019

Is this still a P0?

@mkustermann
Copy link
Member

@rmacnak-google has performed this refactoring :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rmacnak-google rmacnak-google

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P0 A serious issue requiring immediate resolution
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mkustermann @rmacnak-google @athomas