dart2dart: Implementation of simple try/catch.

dart2dart: Implementation of simple try/catch.

A LetHandler form is introduced to the CPS IR.  It binds an exception
handler continuation which is in scope for its body.  LetHandler is
translated to try/catch when translating back to direct style.

Optimizations in the CPS and Tree languages that move code have to be
careful when moving code into or out of the scope of an exception
handler.  This change prohibits such code motion, though it might be
sometimes provably safe.

BUG=
R=asgerf@google.com, karlklose@google.com

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

[Kevin Millikin (Google), 2015-02-13 10:16:04]

kmillikin@google.com changed reviewers:
+ asgerf@google.com, floitsch@google.com, johnniwinther@google.com,
karlklose@google.com

[Kevin Millikin (Google), 2015-02-13 10:16:05]

kmillikin@google.com changed required reviewers:
+ asgerf@google.com, karlklose@google.com

[Kevin Millikin (Google), 2015-02-13 10:16:06]

Here is an initial implementation of try/catch.  Missing is (1) multiple catch
clauses/'on T' clauses, (2) finally, (3) JS backend support.

[karlklose, 2015-02-16 10:15:49]

LGTM.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:561:
catchBuilder.environment.extend(elements[catchClause.exception] as Local,
Why do you cast to Local here and below?

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:157: // Binding an exception
handler.
Make this a DartDoc comment ('///'), and maybe put the TODO outside in a normal
comment.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:272: // site.  This is not
safe if the body is moved inside an exception handler
'inside' -> 'into'?

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:455:
node.handler.parameters.forEach((Parameter p) => setValue(p, nonConst()));
We should set the type of the parameters here.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:103: // The
successor statements for labeled statements that have only one break
Use '///'.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:105: // not
safe if the code would be move inside the scope of an exception
'move' -> 'moved'.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:106: //
handler (i.e., if the code would be moved into a try from outside it.
Missing ')'.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:60: // Code for continuations
with exactly one use is inlined at the use site.
Use '///'.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:145: // It's not obvious how we
want to represent try statements here.
Do you know what V8 or the SSA based backend do? Maybe we should just copy that.

[floitsch, 2015-02-16 14:54:08]

DBC.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:213: /// This include which
variables should be copied into [ir.MutableVariable]s
includes

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2066: void
computeVariablesBoxedOnEntry(Set<LocalVariableElement> boxedInParent) {
Comments.
At the very least mention that the argument is going to be modified (when?).

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2071: // in this try but not
including variables assigned only in some nested
I don't understand this comment.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2076: // enclosing try or boxed
on entry to this try.
The boxed-on-entry happens earlier. It's slightly confusing if you mention it
here.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:501: // of the try block
and at the beginning of the catch block, so th
the

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:502: // variables are
unboxed in the catch block and at the join point.
I guess they are also unboxed before calls to "continue" or "break". Not sure if
it requires an update to the comment.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:512:
tryInfo.boxedOnEntry.removeAll(tryCatchBuilder.mutableCapturedVariables);
I'm not a fan of these kind of side-effects in a builder-visitor.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1311:
node.handler.parameters.forEach((p) {
We tend to type (even for closures) and not to abbreviate.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1313: release(p);
I'm not sure I understand (even with the comment). Maybe create a function that
does the combination and has a clearer name?

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/redundant_phi.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/redundant_phi.dart:90: if (current == letCont)
return !seenHandler;
while (current != letCont) {
  if (current is LetHandler) return false;
  current = current.parent;
}
return true;

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/redundant_phi.dart:124: ref != null;
align with "Reference".

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:566: class TryStatement extends
Statement {
Try should be good enough.

[asgerf, 2015-02-20 10:10:08]

LGTM with comments.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:215: Map<ast.TryStatement,
TryStatementInfo> get tryStatements;
I thought the IR builder was supposed to be independent of the AST so
analyzer2dart could reuse it.

I suspect this will be moved to the IR builder visitor when analyzer2dart wants
to support try.

Just an FYI.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2073: // immediately enclosing
try.
I would remove the word "However" here.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:544: tryBuilder._current
= null;
Would it make sense to extract these 8 lines into
tryBuilder.jumpTo(joinContinuation)?

And similarly catchBuilder.jumpTo(joinContinuation) below.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:369: // of two
(TODO(kmillikin): is that true?).
I agree that they should have two parameters. Please make sure this is the case
and then remove the TODOs.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:276:
safeForHandlers.contains(jump.target)) {
Isn't this a bit expensive? It looks like a quadratic behavior will kick in very
easily.

Wouldn't it be better to use a Set, or at least only add singly-used labels to
the list to make it shorter (so it becomes "safeForInlining").

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:535: labels[cont] = new
Label();
I think you should create the Label inside visitLetHandler. Seems more
consistent since visitLetCont creates its own labels too.

I also think safeForHandlers could be turned into safeForInlining by only adding
singly-used continuations. That should simplify the check here.

[Kevin Millikin (Google), 2015-02-24 11:59:25]

Thanks for the review.  I'm addressing the comments and fixing a bug (this
change does not handle break and continue in the body of a try block to a target
outside the try block).

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:213: /// This include which
variables should be copied into [ir.MutableVariable]s
On 2015/02/16 14:54:07, floitsch wrote:
> includes

Done.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:215: Map<ast.TryStatement,
TryStatementInfo> get tryStatements;
On 2015/02/20 10:10:07, asgerf wrote:
> I thought the IR builder was supposed to be independent of the AST so
> analyzer2dart could reuse it.
> 
> I suspect this will be moved to the IR builder visitor when analyzer2dart
wants
> to support try.
> 
> Just an FYI.

Acknowledged.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2066: void
computeVariablesBoxedOnEntry(Set<LocalVariableElement> boxedInParent) {
On 2015/02/16 14:54:07, floitsch wrote:
> Comments.
> At the very least mention that the argument is going to be modified (when?).

This whole approach is overly complicated.  I've changed it so instead of
building a nesting tree and traversing it afterward, we just maintain a nesting
stack during the analysis.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:502: // variables are
unboxed in the catch block and at the join point.
On 2015/02/16 14:54:07, floitsch wrote:
> I guess they are also unboxed before calls to "continue" or "break". Not sure
if
> it requires an update to the comment.

It does require an update to the comment, and a fix to the implementation
because it's not correct as is.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:512:
tryInfo.boxedOnEntry.removeAll(tryCatchBuilder.mutableCapturedVariables);
On 2015/02/16 14:54:07, floitsch wrote:
> I'm not a fan of these kind of side-effects in a builder-visitor.

Hmmm.  The other approach that we use a lot is to have some kind of predicate
that says "if it's in this set and not in this other set, then box it".  I can
do that instead but I'm not sure it's obviously better.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:544: tryBuilder._current
= null;
On 2015/02/20 10:10:07, asgerf wrote:
> Would it make sense to extract these 8 lines into
> tryBuilder.jumpTo(joinContinuation)?
> 
> And similarly catchBuilder.jumpTo(joinContinuation) below.

Yes.  It also occurs when breaking from a labeled (non-loop) statement.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:561:
catchBuilder.environment.extend(elements[catchClause.exception] as Local,
On 2015/02/16 10:15:48, karlklose wrote:
> Why do you cast to Local here and below?

Otherwise the editor reports "The argument type 'Element' cannot be assigned to
the parameter type 'Local'".

As far as I can tell (this comes from a NodeList so I've got to try to reason
about where it was put into that list) this is always a LocalVariableElement,
which is an Element and a Local.  But Element and Local are not related.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:157: // Binding an exception
handler.
On 2015/02/16 10:15:48, karlklose wrote:
> Make this a DartDoc comment ('///'), and maybe put the TODO outside in a
normal
> comment.

OK.  But I'll just address the TODO before submitting, it was a "draft" TODO :)

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1311:
node.handler.parameters.forEach((p) {
On 2015/02/16 14:54:07, floitsch wrote:
> We tend to type (even for closures) and not to abbreviate.

Done.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1313: release(p);
On 2015/02/16 14:54:07, floitsch wrote:
> I'm not sure I understand (even with the comment). Maybe create a function
that
> does the combination and has a clearer name?

The clearest name I can think of is "AllocateThenRelease" :)  I think a comment
here would have to describe both this algorithm and also what we do with the
result to make sense, and this doesn't seem like the right place for that.

We probably just need comments for functions allocate and release.

====
What's happening here is that this is a backward pass over the CPS term and it
assigns indexes to used variables when it sees their last use, then releases
those indexes when it passes their definitions (moving backwards) because the
index can be used for another variable whose last use occurs before this one's
definition.

Variables that don't get assigned an index must not have had any uses, and so
they are eliminated from the output.

So: after visiting the catch block, if the catch parameters are unused they
still need to be assigned indexes to ensure that they are not eliminated.  And
in any case, they need to be released so that their index can be possibly
reused.

I've rewritten the comment, it's longer but I'm not sure it's any better.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/redundant_phi.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/redundant_phi.dart:90: if (current == letCont)
return !seenHandler;
On 2015/02/16 14:54:07, floitsch wrote:
> while (current != letCont) {
>   if (current is LetHandler) return false;
>   current = current.parent;
> }
> return true;

That's not quite the same.  It can be the case that uniqueDefinition is bound
outside letCont, in that case we will not move uniqueDefinition and it is safe.

So we need to check: is letCont on the path to the root and do we see a handler
before seeing letCont.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/redundant_phi.dart:124: ref != null;
On 2015/02/16 14:54:07, floitsch wrote:
> align with "Reference".

Done.

[Kevin Millikin (Google), 2015-02-25 11:06:37]

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:272: // site.  This is not
safe if the body is moved inside an exception handler
On 2015/02/16 10:15:48, karlklose wrote:
> 'inside' -> 'into'?

Thanks.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:369: // of two
(TODO(kmillikin): is that true?).
On 2015/02/20 10:10:07, asgerf wrote:
> I agree that they should have two parameters. Please make sure this is the
case
> and then remove the TODOs.

Done.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:455:
node.handler.parameters.forEach((Parameter p) => setValue(p, nonConst()));
On 2015/02/16 10:15:48, karlklose wrote:
> We should set the type of the parameters here.

Acknowledged.  I've made it a TODO because the way we handle the exception
parameter will depend on how we handle 'on T' catch clauses in a later change.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:106: //
handler (i.e., if the code would be moved into a try from outside it.
On 2015/02/16 10:15:48, karlklose wrote:
> Missing ')'.

That's embarassing for a Lisper.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:276:
safeForHandlers.contains(jump.target)) {
On 2015/02/20 10:10:07, asgerf wrote:
> Isn't this a bit expensive? It looks like a quadratic behavior will kick in
very
> easily.
> 
> Wouldn't it be better to use a Set, or at least only add singly-used labels to
> the list to make it shorter (so it becomes "safeForInlining").

I don't think it's best to add only singly-used labels, since it's difficult to
reason about the way that we adjust the use counts of labels on the fly.

I'll change it to a Set, though that might have a higher cost than List for
'pushing' and 'popping'.  I suppose it's not necessary to ever remove labels
from the set --- do you think it's a good idea not to remove them?

I'll still change the name to safeForInlining, because that's better (having
multiple uses doesn't make a label 'unsafe' anyway).

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:535: labels[cont] = new
Label();
On 2015/02/20 10:10:08, asgerf wrote:
> I think you should create the Label inside visitLetHandler. Seems more
> consistent since visitLetCont creates its own labels too.
> 
> I also think safeForHandlers could be turned into safeForInlining by only
adding
> singly-used continuations. That should simplify the check here.

I agree that the whole thing is complicated.

* This is a label for a singly-used continuation bound by LetCont.

* visitLetCont uses the existence of a label to detect whether the continuation
was inlined (I don't think it actually needed to be that way before, we chould
have just used hasExactlyOneUse).

* We can't easily tell in visitLetCont for singly-used continuations whether
their use will be safe to inline or not.

We could be consistent by always generating labels for multiply-used
continuations when we encounter their first invocation, but it looks like there
are a lot of places where that happens so I'm not sure if that's better.

Do you think we should just always generate labels in visitLetCont, and use
something like the label's use count to see if it was inlined?

I've changed the name to safeForInlining.  They were already singly-used.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:566: class TryStatement extends
Statement {
On 2015/02/16 14:54:07, floitsch wrote:
> Try should be good enough.

Done.

[asgerf, 2015-02-25 11:55:11]

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:276:
safeForHandlers.contains(jump.target)) {
On 2015/02/25 11:06:36, kmillikin wrote:
> On 2015/02/20 10:10:07, asgerf wrote:
> > Isn't this a bit expensive? It looks like a quadratic behavior will kick in
> very
> > easily.
> > 
> > Wouldn't it be better to use a Set, or at least only add singly-used labels
to
> > the list to make it shorter (so it becomes "safeForInlining").
> 
> I don't think it's best to add only singly-used labels, since it's difficult
to
> reason about the way that we adjust the use counts of labels on the fly.
> 
> I'll change it to a Set, though that might have a higher cost than List for
> 'pushing' and 'popping'.  I suppose it's not necessary to ever remove labels
> from the set --- do you think it's a good idea not to remove them?
> 
> I'll still change the name to safeForInlining, because that's better (having
> multiple uses doesn't make a label 'unsafe' anyway).

Ah, I forgot the use-counts on labels change during this phase.

I like to remove garbage from maps and sets when it's easy to do so. It usually
pays off.

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/923013002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:535: labels[cont] = new
Label();
On 2015/02/25 11:06:36, kmillikin wrote:
> On 2015/02/20 10:10:08, asgerf wrote:
> > I think you should create the Label inside visitLetHandler. Seems more
> > consistent since visitLetCont creates its own labels too.
> > 
> > I also think safeForHandlers could be turned into safeForInlining by only
> adding
> > singly-used continuations. That should simplify the check here.
> 
> I agree that the whole thing is complicated.
> 
> * This is a label for a singly-used continuation bound by LetCont.
> 
> * visitLetCont uses the existence of a label to detect whether the
continuation
> was inlined (I don't think it actually needed to be that way before, we chould
> have just used hasExactlyOneUse).
> 
> * We can't easily tell in visitLetCont for singly-used continuations whether
> their use will be safe to inline or not.
> 
> We could be consistent by always generating labels for multiply-used
> continuations when we encounter their first invocation, but it looks like
there
> are a lot of places where that happens so I'm not sure if that's better.
> 
> Do you think we should just always generate labels in visitLetCont, and use
> something like the label's use count to see if it was inlined?
> 
> I've changed the name to safeForInlining.  They were already singly-used.

I guess we could also just let the statement rewriter do all the inlining, since
it's already doing that. There might a performance hit, though, and the output
from the tree tracer (before rewriting) would likely become unreadable.

Otherwise, just make it a bit less subtle and always create labels on-demand or
always ahead-of-time. Always on-demand seems simplest, though I may have
overlooked something:

if (safeForInlining.contains(cont)) {
    return visit(cont.body)
} else {
    Label label = labels.putIfAbsent(cont, () => new Label());
    return new Break(label);
}

[Kevin Millikin (Google), 2015-02-25 16:08:36]

I have a new patch that is enough different that it needs a fresh look:

1. The complicated variable analysis is substantially simplified.  Instead of
building a forest representing the try nesting structure and processing it
afterward, we just maintain a stack of enclosing try blocks at analysis time. 
Look in class DartCapturedVariables in cps_ir/cps_ir_builder_visitor.dart

2. Break and continue from within try blocks is handled.  As an overview of
break and continue in general: we have a class JumpCollector that collects a
list of all the break or continue sites, and we process them after translating
all the places that could contain them.  Now: when entering a try block, this
class records the mutable variables that have to have their values passed to the
target of the break or continue (and they're removed afterward --- the whole
JumpCollector class is stateful).  Look in class JumpCollector in
cps_ir/cps_ir_builder.dart and in visitTryStatement in the builder visitor in
cps_ir/cps_ir_builder_visitor.dart.

[asgerf, 2015-02-26 10:30:11]

LGTM!

[karlklose, 2015-02-26 11:24:23]

SLGTM.

https://codereview.chromium.org/923013002/diff/20001/pkg/compiler/lib/src/dar...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/923013002/diff/20001/pkg/compiler/lib/src/dar...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:637:
BuilderContext<Statement> context) {
Align parameters.

[Kevin Millikin (Google), 2015-02-26 12:47:06]

https://codereview.chromium.org/923013002/diff/20001/pkg/compiler/lib/src/dar...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/923013002/diff/20001/pkg/compiler/lib/src/dar...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:637:
BuilderContext<Statement> context) {
On 2015/02/26 11:24:23, karlklose wrote:
> Align parameters.

Done.

[Kevin Millikin (Google), 2015-02-26 13:25:33]

Committed patchset #2 (id:20001) manually as 44047 (presubmit successful).