dart2dart: Restore loops with conditions and updates in backend.

dart2dart: Restore loops with conditions and updates in backend.

BUG=
R=kmillikin@google.com, sigurdm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37266

[Kevin Millikin (Google), 2014-06-10 11:08:31]

LGTM.  I had two small questions when reading the code and so it probably needs
comments or slight restructuring to make the answers clear.

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1333:
body.thenStatement = visitStatement(body.thenStatement);
I can't guess how it would happen, but we should assert that usedContinueLabels
does not include node.label before calling visitStatement here, if only to make
the intent clear.

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1361:
Statement visitWhileCondition(WhileCondition node) {
Can this method be called?  Does the input language have WhileCondition?

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1376:
target.updates.add(node.expression);
updates will always be an empty list or a singleton, right?

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:504: 
Inadvertent change?

[asgerf, 2014-06-10 12:02:21]

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1333:
body.thenStatement = visitStatement(body.thenStatement);
On 2014/06/10 11:08:31, kmillikin wrote:
> I can't guess how it would happen, but we should assert that
usedContinueLabels
> does not include node.label before calling visitStatement here, if only to
make
> the intent clear.

Done.

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1361:
Statement visitWhileCondition(WhileCondition node) {
On 2014/06/10 11:08:30, kmillikin wrote:
> Can this method be called?  Does the input language have WhileCondition?

No, but the implementation was so trivial that we might as well have it. Also, I
try to write the phases so they are idempotent.

I've left a comment indicating that the code is not called.

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1376:
target.updates.add(node.expression);
On 2014/06/10 11:08:31, kmillikin wrote:
> updates will always be an empty list or a singleton, right?

No, the same pattern might have clicked in the call to
visitStatement(node.next), which would then return a Continue statement,
allowing the pattern to reclick here. I've inserted a comment to indicate this.

But there was actually a bug here because the order of evaluation gets reversed.
I fixed the bug.

I've also added a test case for this.

Right now update expressions aren't that interesting because assignments (such
as ++i) are statements, hence cannot yet be moved into an update. Also, having
multiple updates is at best marginally better than expression statements, so
it's not clear if this is helping at all. For now, I'll just rewrite as
aggressively as possible to see what happens when are able to run on real code.

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/318723002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:504: 
On 2014/06/10 11:08:31, kmillikin wrote:
> Inadvertent change?

Reverted.

[sigurdm, 2014-06-10 12:30:06]

LGTM

https://codereview.chromium.org/318723002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart
(right):

https://codereview.chromium.org/318723002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart:215:
tree.Statement fall = fallthrough;
Why is this in a local?

[asgerf, 2014-06-10 12:46:42]

https://codereview.chromium.org/318723002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart
(right):

https://codereview.chromium.org/318723002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart:215:
tree.Statement fall = fallthrough;
On 2014/06/10 12:30:06, sigurdm wrote:
> Why is this in a local?

Because type promotion needs to kick in when we say "fall is tree.Continue".

[sigurdm, 2014-06-10 12:52:24]

Ah - of course

[asgerf, 2014-06-12 12:48:02]

Committed patchset #6 manually as r37266 (presubmit successful).