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: Implement "Control Flow Collections" #36214

Closed
1 task done
a-siva opened this issue Mar 14, 2019 · 11 comments
Closed
1 task done

VM: Implement "Control Flow Collections" #36214

a-siva opened this issue Mar 14, 2019 · 11 comments
Assignees
@aartbik
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone
Dart 2.3

Comments

@a-siva
Copy link
Contributor

a-siva commented Mar 14, 2019 •
edited by aartbik
Loading

Initially these features are behind the experimental flag control-flow-collections.

  • Support new block expression AST node

See:

Language tracking issue: dart-lang/language#165
Feature specification
Implementation plan

PENDING Unified Collections: dart-lang/language#200
@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Mar 14, 2019
@a-siva a-siva added this to the Dart2.3 milestone Mar 14, 2019
@munificent munificent mentioned this issue Mar 14, 2019
Closed
18 tasks
@aartbik
Copy link
Contributor

aartbik commented Mar 14, 2019

Just for proper book-keeping, the following CL provides an initial implementation of block expressions in the Dart VM:

https://dart-review.googlesource.com/c/sdk/+/94441

This should provide a fully functional implementation to continue testing, even though it still has some issues that could be improved (e.g. for now we just disable OSR completely while running control flow collections code for simplicity; this may be unacceptable in the long run).

@aartbik
Copy link
Contributor

aartbik commented Mar 15, 2019

Current status of control flow collections tests

             control-flow
JIT        +  190 | -   76
AOT        +  190 | -   76

@aartbik
Copy link
Contributor

aartbik commented Mar 25, 2019 •
edited
Loading

Some CFE CLs related to if-statements were submitted:

https://dart-review.googlesource.com/c/sdk/+/97200
https://dart-review.googlesource.com/c/sdk/+/97310

for-loops still pending:

https://dart-review.googlesource.com/c/sdk/+/97628

@aartbik
Copy link
Contributor

aartbik commented Mar 25, 2019 •
edited
Loading

New status (AOT change is probably due to changes to status file beyond my skip/no-skip change).

             control-flow
JIT         +  203 | -   63
AOT         +  186 | -   80

@a-siva a-siva added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Mar 28, 2019
@aartbik
Copy link
Contributor

aartbik commented Mar 28, 2019

Found #36377

@aartbik
Copy link
Contributor

aartbik commented Mar 28, 2019

Also #36357 and #36356 are blocking fuzz test runs

@aartbik
Copy link
Contributor

aartbik commented Mar 29, 2019

This morning's status:

             control-flow
JIT       +  251 | -   15
AOT       +  251 | -   15

@aartbik
Copy link
Contributor

aartbik commented Mar 29, 2019

Probably biggest VM item left to be explored is enabling OSR points inside control flow collection for loops. Alex notes we should ensure that temporaries on the expression stack are preserved when OSR happens.

Consider a more complex expression like

var x = x.bar(42, bazz(), 'abc', [more_bazz(), for (int i = 0; i < n; i++) 2*i ]);

In this case x, 42, bazz() and 'abc' should be on the expression stack unless CFE introduces temporary variables for them. Let's:
(1) test if the above example works;
(2) dump kernel to see how CFE generates this;
(3) print flowgraph of a function compiled for OSR and check that it loads captured values correctly.

@a-siva
Copy link
Contributor Author

a-siva commented Apr 1, 2019

If there is no more compiler backend work left to be done on this we can close the issue, a new issue can be opened to track the OSR optimization and it will not be a blocker for the 2.3 release.

@aartbik
Copy link
Contributor

aartbik commented Apr 1, 2019

Sounds good. I will do that.

@aartbik
Copy link
Contributor

aartbik commented Apr 1, 2019

Tracking OSR follow up in #36421
Closing this, since there is no known VM work left.

@aartbik aartbik closed this as completed Apr 1, 2019
dart-bot pushed a commit that referenced this issue Apr 2, 2019
@alexmarkov
[vm/bytecode] Preliminary support for new language features in bytecode
8fb77ec 
Issue: #36214
Issue: #36218

Change-Id: I39149b82cb93c1cb87c64cf9c41c56b753bba13a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98400
Reviewed-by: Aart Bik <ajcbik@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Auto-Submit: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue Apr 4, 2019
@aartbik
[dart/fuzzer] Fuzz support for spread and control flow collections
5e8b0b7 
Rationale:
Follows the documented grammar to allow for arbitary nested
spead/if/for inside collections (list, set, maps).

#36218
#36214

Change-Id: Ie5b5828a323401c4db14045e6edd0587677f2c19
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98261
Reviewed-by: Aart Bik <ajcbik@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Aart Bik <ajcbik@google.com>
dart-bot pushed a commit that referenced this issue Apr 10, 2019
@aartbik
[dart/vm] Support OSR on non-empty expression stack
9876e92 
Rationale:
This enables OSR even in the presence of block expressions,
which are introduced by the new CFE control-flow collections
and spread operations. Faster is better!

#36421
#36218
#36214

Change-Id: I363a0b0a5becbbd5743f8fb47ff24dcebc0e4d64
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98744
Commit-Queue: Aart Bik <ajcbik@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue Apr 18, 2019
@aartbik
[dart/vm] Reduce trip count of osr test
3cf3e4e 
Rationale:
Test was borderline timeout. The reduced tripcount
still triggers OSR consistently, but runs a lot faster.

#36421
#36218
#36214

Change-Id: I60917bb6bdd51dee6bd11d1fe5beede4753c2147
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99783
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Aart Bik <ajcbik@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aartbik aartbik

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Milestone
Dart 2.3
Development

No branches or pull requests
2 participants
@a-siva @aartbik