Optimize functions containing try-catch.

Optimize functions containing try-catch.

This is a first step towards fully optimizing try-catch-finally.

At a catch entry, all local variables and parameters are
expected at a fixed stack location. There is a list of
initial definitions at the catch entry block, similar to
the initial definitions at graph entry.

Inside every try-block there is a special prologue code before each
call (instruction that may throw) inside the try-block. This prologue
is similar to a parallel move instruction: It moves all locals+parameters
to the locations expected by the catch-entry block. The stack frame
is extended with the corresponding number of fixed slots right below
the normal spill slots.

Every function containing try-catch has additional compiler-
generated local variables to pass the context, the exception and
the stack trace.

Variable liveness analysis is adapted to treat locals inside try{} blocks
specially: Every call has an implicit LoadLocal of every local variable.
This CL uses a safe approximiation of liveness which can be optimized further.

Current restrictions which are planned for future CLs:
 * No inlining inside try-blocks.
 * No inlining of functions containing try-catch.
 * No try-finally yet.

R=kmillikin@google.com

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

[srdjan, 2013-05-07 17:34:05]

Instead of ifdef ARM/MIPS you could add a flag --bailout_on_try_catch and set it
to false for ia32/x64. That way, in a (unlikely) situation that there are
problems we could easily turn off optimization  of try-catch without having to
revert.

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:12: #include "vm/il_printer.h"
Remove.

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:382: 
Two lines space seems too match within a function (also some places below).

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph_bui...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph_bui...
runtime/vm/flow_graph_builder.cc:3075:
Bailout("EffectGraphVisitor::VisitCatchClauseNode (exception)");
Why is this bailout necessary?

https://codereview.chromium.org/14682020/diff/29004/tests/language/try_catch3...
File tests/language/try_catch3_test.dart (right):

https://codereview.chromium.org/14682020/diff/29004/tests/language/try_catch3...
tests/language/try_catch3_test.dart:116: for (var i = 0; i < 2000; i++) {
Optional: you can make the loop smaller and add optimization-counter-threshold=5
as VMOption

[Florian Schneider, 2013-05-08 08:57:47]

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:12: #include "vm/il_printer.h"
On 2013/05/07 17:34:05, srdjan wrote:
> Remove.

Done.

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:382: 
On 2013/05/07 17:34:05, srdjan wrote:
> Two lines space seems too match within a function (also some places below).

Done.

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph_bui...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/14682020/diff/29004/runtime/vm/flow_graph_bui...
runtime/vm/flow_graph_builder.cc:3075:
Bailout("EffectGraphVisitor::VisitCatchClauseNode (exception)");
On 2013/05/07 17:34:05, srdjan wrote:
> Why is this bailout necessary?

Oops,  yes this is the wrong place here. I was incorrectly assuming that Bailout
only affects the optimizing backend.

As you suggested I'll remove it and add a flag to enable/disable optimizing
try-catch for ARM and MIPS.

[Kevin Millikin (Google), 2013-05-08 11:42:00]

I think the approach will work.

I'm sure all the special handing is necessary, but it's not obvious why and what
would break if we didn't have it.  The comments definitely need to say 'why' not
'what' and to be concrete.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:336: // All locals are assigned inside try-catch.
Comment needs to say why.  I don't understand why it must be the case that all
locals are assigned in every block inside a try.

Also: be precise in the comments about the difference between blocks inside the
try, blocks inside the catch, the try entry, and the catch entry.  It can be
ambiguous to say simply 'inside try-catch'.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:391: // Inside try-catch all locals are live.
Again, I don't understand why.  This is just for all the blocks occurring inside
the try, right?

Is this an approximation because we don't have edges from the calls to the catch
block?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:426: if (block->try_index() ==
CatchClauseNode::kInvalidTryIndex) {
This comparison is always true.  The loop is not entered in the case that the
condition is false.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:715: if (block_entry->try_index() !=
CatchClauseNode::kInvalidTryIndex) {
This comparison is repeated a lot in this change.  It's more readable to
introduce a predicate on blocks IsInsideTry, or NestedInsideTry, or the like.

We also need the comment to record why all phis inside blocks inside the try are
always live, even if not otherwise.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:724: // parameters and locals is the same as at
function entry.
I don't think this comment is quite correct.  The locals are all uninitialized
at function entry and do not have distinct locations, do they?

I think you mean that the location is the same as in the unoptimized stack
frame, though I'm not sure that's important here.  Nothing about this place has
anything to do with locations anyway.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:790: store->local().always_live()) {
Need to know why, also below.

These are for the the three special catch variables, right?  Do we have stores
to them that have implicit loads (so they aren't reflected in liveness analysis)
and should not be eliminated?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:809: !load->local().always_live()) {
Is this because there are implicit loads from the special catch variables, so
this might not really be the last load?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_all...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_all...
runtime/vm/flow_graph_allocator.cc:187: // Process initial definitions.
It's disconcerting that locals are treated so differently in the two different
liveness analyses.  In the other one, upwards exposed uses of variables in the
catch body add those variables to the live_in set.  In this one, the live_in set
is empty?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_all...
runtime/vm/flow_graph_allocator.cc:515: Definition* defn =
(*catch_entry->initial_definitions())[i];
All this code that is duplicated below should be moved into a function
ProcessInitialDefinition or the like.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler.cc:320: GraphEntryInstr* entry =
block_order_[0]->AsGraphEntry();
Not your code, but I don't like using a property of the block order (which could
change) to find the graph entry.  Replace with flow_graph_.graph_entry().

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:667: if ((*idefs)[i]->IsConstant())
continue;  // common constants
Common constants.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:670: Address dest(EBP,
(kLastParamSlotIndex + index - 1) * kWordSize);
I don't like the name "index" --- it's not what I think of as the index (it's
the 1-based parameter index when numbered in reverse?) and that makes it
confusing.

Since the subexpression is

kLastParamSlotIndex + flow_graph().num_non_copied_params() - i - 1

you could refactor out the unchanging parts and write:

const intptr_t base =
    kLastParamSlotIndex + flow_graph().num_non_copied_params() - 1;
Address dest(EBP, (base - i) * kWordSize);

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:679: __ movl(dest, loc.reg());
Is this safe?  What if loc.reg() is EAX and push_emitted is true?  In that case
don't you want

movl(EAX, Address(ESP, 0));
movl(dest, EAX);

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:694: intptr_t nncp =
flow_graph_.num_non_copied_params();
Yucky name.  What about:

intptr_t base =
    kFirstLocalSlotIndex + flow_graph_.num_non_copied_params();
intptr_t ex_idx = base - catch_entry->exception_var().index();
intptr_t st_idx = base - catch_entry->stacktrace_var().index();

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:704: Address dest(EBP,
(kFirstLocalSlotIndex - index) * kWordSize);
If you introduce 'base' as suggested above, this becomes

Address dest(EBP, (base - i) * kWordSize);

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:705: if (loc.IsConstant()) {
Introduce a function that takes an Address and in-out pointer to push_emitted,
and emits the move code.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:653: void
FlowGraphCompiler::EmitTrySync(Instruction* instr, intptr_t try_index) {
Same comments as IA32.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:2690: void RangeAnalysis::CollectSmiValues()
{
Note that this function does not handle smi constant in the catch block initial
definitions.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3061: void
RangeAnalysis::InferRangesRecursive(BlockEntryInstr* block) {
Note that this function does not infer ranges for constants in the catch entry's
initial definitions.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3148: void
FlowGraphOptimizer::AnalyzeTryCatch() {
Doesn't need to be in the FlowGraphOptimizer, which is a visitor that does
ApplyICData.

I think it's confusing to put too many unrelated passes into that same visitor
class.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3155: intptr_t nncp =
flow_graph_->num_non_copied_params();
intptr_t base = kFirstLocalSlotIndex + flow_graph_->num_non_copied_params();

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3161: intptr_t ex_idx =
intptr_t ex_idx = base - catch_entry->exception_var().index();
intptr_t st_idx = base - catch_entry->stacktrace_var().index();

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3169: for (BlockIterator block_it =
flow_graph_->reverse_postorder_iterator();
This is doing constant analysis with the lattice:

known to be non-constant (rep: NULL)
  |
known constant (rep: ConstantInstr)
  |
unknown constant (rep: Parameter Instr)

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3183: cdefs[env_idx]->AsParameter()->index()
!= ex_idx &&
The intent is that ex_idx and st_idx are not constant.  I think it's simpler to
set them to NULL before performing the analysis and have no special treatment in
the loop.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3185:
env->ValueAt(env_idx)->definition()->IsConstant()) {
I guess you can write this as env->ValueAt(env_idx)->BindsToConstant();

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3200: ConstantInstr* copy = new
ConstantInstr(orig->value());
Note that all constants are pooled now.  I think you can simply replaces uses
with the one from the pool and remove this definition from the catch block's
initial definitions.  Will that work?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:4473: SetReachable(block->SuccessorAt(i));
Hmm, this is a poor approximation.  Can you add a comment that a better one is
that the catch is reachable if the try entry is (actually, if a call inside the
try is).

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_typ...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_typ...
runtime/vm/flow_graph_type_propagator.cc:672: if (graph_entry == NULL) return
CompileType::Dynamic();
Comment that this is for the parameters at a catch entry, which have type
dynamic.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:245: for (intptr_t i = 0; i <
catch_entries_.length(); ++i) {
Would it be simple to just sort these by try index and index them directly
instead of searching?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:829: virtual bool MayThrow() const  = 0;
There's an extra space after const for some reason.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:6024: context_var->mark_as_always_live();
Eeek, what a hack (to set a bit here in the parser, so we can have special
handling in the optimizing compiler liveness analysis).

We should probably come up with a way to handle it within the system.  Maybe we
just need to expose the stores to the variables that are implicit in the catch
entry?

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/scopes.h#newco...
runtime/vm/scopes.h:76: bool always_live() const { return always_live_; }
This needs a comment that is is, for example, for things like the catch context
variable, catch stack trace variable, etc.

[Florian Schneider, 2013-05-08 17:10:55]

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:336: // All locals are assigned inside try-catch.
On 2013/05/08 11:42:00, kmillikin wrote:
> Comment needs to say why.  I don't understand why it must be the case that all
> locals are assigned in every block inside a try.
> 
> Also: be precise in the comments about the difference between blocks inside
the
> try, blocks inside the catch, the try entry, and the catch entry.  It can be
> ambiguous to say simply 'inside try-catch'.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:391: // Inside try-catch all locals are live.
On 2013/05/08 11:42:00, kmillikin wrote:
> Again, I don't understand why.  This is just for all the blocks occurring
inside
> the try, right?
> 
> Is this an approximation because we don't have edges from the calls to the
catch
> block?

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:426: if (block->try_index() ==
CatchClauseNode::kInvalidTryIndex) {
On 2013/05/08 11:42:00, kmillikin wrote:
> This comparison is always true.  The loop is not entered in the case that the
> condition is false.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:715: if (block_entry->try_index() !=
CatchClauseNode::kInvalidTryIndex) {
On 2013/05/08 11:42:00, kmillikin wrote:
> This comparison is repeated a lot in this change.  It's more readable to
> introduce a predicate on blocks IsInsideTry, or NestedInsideTry, or the like.
> 
> We also need the comment to record why all phis inside blocks inside the try
are
> always live, even if not otherwise.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:724: // parameters and locals is the same as at
function entry.
On 2013/05/08 11:42:00, kmillikin wrote:
> I don't think this comment is quite correct.  The locals are all uninitialized
> at function entry and do not have distinct locations, do they?
> 
> I think you mean that the location is the same as in the unoptimized stack
> frame, though I'm not sure that's important here.  Nothing about this place
has
> anything to do with locations anyway.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:790: store->local().always_live()) {
On 2013/05/08 11:42:00, kmillikin wrote:
> Need to know why, also below.
> 
> These are for the the three special catch variables, right?  Do we have stores
> to them that have implicit loads (so they aren't reflected in liveness
analysis)
> and should not be eliminated?

Done. Not needed anymore.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:809: !load->local().always_live()) {
On 2013/05/08 11:42:00, kmillikin wrote:
> Is this because there are implicit loads from the special catch variables, so
> this might not really be the last load?

Done. Not needed anymore.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_all...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_all...
runtime/vm/flow_graph_allocator.cc:187: // Process initial definitions.
On 2013/05/08 11:42:00, kmillikin wrote:
> It's disconcerting that locals are treated so differently in the two different
> liveness analyses.  In the other one, upwards exposed uses of variables in the
> catch body add those variables to the live_in set.  In this one, the live_in
set
> is empty?

This deals with SSA values, similar to the initial definitions of the graph
entry.

The other one is changed to correctly deal with local variables inside try{}. I
simplified the changes to the variable liveness analysis.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_all...
runtime/vm/flow_graph_allocator.cc:515: Definition* defn =
(*catch_entry->initial_definitions())[i];
On 2013/05/08 11:42:00, kmillikin wrote:
> All this code that is duplicated below should be moved into a function
> ProcessInitialDefinition or the like.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler.cc:320: GraphEntryInstr* entry =
block_order_[0]->AsGraphEntry();
On 2013/05/08 11:42:00, kmillikin wrote:
> Not your code, but I don't like using a property of the block order (which
could
> change) to find the graph entry.  Replace with flow_graph_.graph_entry().

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:667: if ((*idefs)[i]->IsConstant())
continue;  // common constants
On 2013/05/08 11:42:00, kmillikin wrote:
> Common constants.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:670: Address dest(EBP,
(kLastParamSlotIndex + index - 1) * kWordSize);
On 2013/05/08 11:42:00, kmillikin wrote:
> I don't like the name "index" --- it's not what I think of as the index (it's
> the 1-based parameter index when numbered in reverse?) and that makes it
> confusing.
> 
> Since the subexpression is
> 
> kLastParamSlotIndex + flow_graph().num_non_copied_params() - i - 1
> 
> you could refactor out the unchanging parts and write:
> 
> const intptr_t base =
>     kLastParamSlotIndex + flow_graph().num_non_copied_params() - 1;
> Address dest(EBP, (base - i) * kWordSize);

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:679: __ movl(dest, loc.reg());
On 2013/05/08 11:42:00, kmillikin wrote:
> Is this safe?  What if loc.reg() is EAX and push_emitted is true?  In that
case
> don't you want
> 
> movl(EAX, Address(ESP, 0));
> movl(dest, EAX);

Done. Yes, of course. That is a bug. I wonder why no test hit that case.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:694: intptr_t nncp =
flow_graph_.num_non_copied_params();
On 2013/05/08 11:42:00, kmillikin wrote:
> Yucky name.  What about:
> 
> intptr_t base =
>     kFirstLocalSlotIndex + flow_graph_.num_non_copied_params();
> intptr_t ex_idx = base - catch_entry->exception_var().index();
> intptr_t st_idx = base - catch_entry->stacktrace_var().index();

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:704: Address dest(EBP,
(kFirstLocalSlotIndex - index) * kWordSize);
On 2013/05/08 11:42:00, kmillikin wrote:
> If you introduce 'base' as suggested above, this becomes
> 
> Address dest(EBP, (base - i) * kWordSize);

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:705: if (loc.IsConstant()) {
On 2013/05/08 11:42:00, kmillikin wrote:
> Introduce a function that takes an Address and in-out pointer to push_emitted,
> and emits the move code.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:653: void
FlowGraphCompiler::EmitTrySync(Instruction* instr, intptr_t try_index) {
On 2013/05/08 11:42:00, kmillikin wrote:
> Same comments as IA32.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:2690: void RangeAnalysis::CollectSmiValues()
{
On 2013/05/08 11:42:00, kmillikin wrote:
> Note that this function does not handle smi constant in the catch block
initial
> definitions.

Good catch. Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3061: void
RangeAnalysis::InferRangesRecursive(BlockEntryInstr* block) {
On 2013/05/08 11:42:00, kmillikin wrote:
> Note that this function does not infer ranges for constants in the catch
entry's
> initial definitions.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3148: void
FlowGraphOptimizer::AnalyzeTryCatch() {
On 2013/05/08 11:42:00, kmillikin wrote:
> Doesn't need to be in the FlowGraphOptimizer, which is a visitor that does
> ApplyICData.
> 
> I think it's confusing to put too many unrelated passes into that same visitor
> class.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3155: intptr_t nncp =
flow_graph_->num_non_copied_params();
On 2013/05/08 11:42:00, kmillikin wrote:
> intptr_t base = kFirstLocalSlotIndex + flow_graph_->num_non_copied_params();

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3161: intptr_t ex_idx =
On 2013/05/08 11:42:00, kmillikin wrote:
> intptr_t ex_idx = base - catch_entry->exception_var().index();
> intptr_t st_idx = base - catch_entry->stacktrace_var().index();

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3169: for (BlockIterator block_it =
flow_graph_->reverse_postorder_iterator();
On 2013/05/08 11:42:00, kmillikin wrote:
> This is doing constant analysis with the lattice:
> 
> known to be non-constant (rep: NULL)
>   |
> known constant (rep: ConstantInstr)
>   |
> unknown constant (rep: Parameter Instr)

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3183: cdefs[env_idx]->AsParameter()->index()
!= ex_idx &&
On 2013/05/08 11:42:00, kmillikin wrote:
> The intent is that ex_idx and st_idx are not constant.  I think it's simpler
to
> set them to NULL before performing the analysis and have no special treatment
in
> the loop.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3185:
env->ValueAt(env_idx)->definition()->IsConstant()) {
On 2013/05/08 11:42:00, kmillikin wrote:
> I guess you can write this as env->ValueAt(env_idx)->BindsToConstant();

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:3200: ConstantInstr* copy = new
ConstantInstr(orig->value());
On 2013/05/08 11:42:00, kmillikin wrote:
> Note that all constants are pooled now.  I think you can simply replaces uses
> with the one from the pool and remove this definition from the catch block's
> initial definitions.  Will that work?

Yes. It should and make the code much simpler here. Added a TODO for now.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:4473: SetReachable(block->SuccessorAt(i));
On 2013/05/08 11:42:00, kmillikin wrote:
> Hmm, this is a poor approximation.  Can you add a comment that a better one is
> that the catch is reachable if the try entry is (actually, if a call inside
the
> try is).

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_typ...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/flow_graph_typ...
runtime/vm/flow_graph_type_propagator.cc:672: if (graph_entry == NULL) return
CompileType::Dynamic();
On 2013/05/08 11:42:00, kmillikin wrote:
> Comment that this is for the parameters at a catch entry, which have type
> dynamic.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:245: for (intptr_t i = 0; i <
catch_entries_.length(); ++i) {
On 2013/05/08 11:42:00, kmillikin wrote:
> Would it be simple to just sort these by try index and index them directly
> instead of searching?

Possibly. I'll leave it for now since I don't see more than a couple of those
per function.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:829: virtual bool MayThrow() const  = 0;
On 2013/05/08 11:42:00, kmillikin wrote:
> There's an extra space after const for some reason.

Done.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:6024: context_var->mark_as_always_live();
On 2013/05/08 11:42:00, kmillikin wrote:
> Eeek, what a hack (to set a bit here in the parser, so we can have special
> handling in the optimizing compiler liveness analysis).
> 
> We should probably come up with a way to handle it within the system.  Maybe
we
> just need to expose the stores to the variables that are implicit in the catch
> entry?

Removed. The conservative approximation in the variable liveness analysis makes
this flag obsolete.

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/14682020/diff/40001/runtime/vm/scopes.h#newco...
runtime/vm/scopes.h:76: bool always_live() const { return always_live_; }
On 2013/05/08 11:42:00, kmillikin wrote:
> This needs a comment that is is, for example, for things like the catch
context
> variable, catch stack trace variable, etc.

Done.

[Kevin Millikin (Google), 2013-05-13 09:42:06]

Comments look much better.

LGTM.

[Florian Schneider, 2013-05-13 10:33:44]

Committed patchset #8 manually as r22615 (presubmit successful).