Shuffle around deopt id allocation to give the flow graph builder a chance to record other data as …

Shuffle around deopt id allocation to give the flow graph builder a chance to record other data as they are allocated.

We'll use deopt-id -> context-level or deopt-id -> scope mappings to find what variables are in scope for the debugger and async stack trace machinery.

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/c94bebf28937e5250b2d6486503848645be142b1

[rmacnak, 2017-05-22 20:11:01]

rmacnak@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2017-05-23 12:00:03]

LGTM!

You will see that my comments are mostly repetitive about not assigning new
deoptimization ids when they are not needed/meaningless or will be (must be)
overwritten.

Also if possible please update CL description to elaborate a bit about which
kind of information you are going to record and how this refactoring facilitates
it. Otherwise it is hard to figure out the intent.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:720: new (Z) Value(right), false,  // No number
check.
better maybe write this comment /* number_check = */ false

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:721: Thread::Current()->GetNextDeoptId());
I think this can be kNoDeoptId

We should probably find a way to stop using deopt_ids altogether in the AOT
compiler as they are not doing anything.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1455: Thread::Current()->GetNextDeoptId());
Ditto.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
File runtime/vm/branch_optimizer.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:70: Thread::Current()->GetNextDeoptId());
This can be Thread::kNoDeoptId because we copy the correct deopt id down below
in the InheritDeoptTarget.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:87: BranchInstr(new_comparison,
Thread::Current()->GetNextDeoptId());
This should probably be Thread::kNoDeoptId

(e.g. it does not make much sense to assign new deoptimization ids to anything
created during optimizations - because they don't "lead" anywhere)

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:190: Thread::Current()->GetNextDeoptId());
kNoDeoptId here, because it is overwritten anyway by InheritDeoptTarget.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:193: TargetEntryInstr(flow_graph->max_block_id()
+ 2, block->try_index(),
ditto

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:200: GotoInstr(join_true,
Thread::Current()->GetNextDeoptId());
ditto

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:205: GotoInstr(join_false,
Thread::Current()->GetNextDeoptId());
ditto

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/branch_optim...
runtime/vm/branch_optimizer.cc:306: Thread::Current()->GetNextDeoptId());
I think this should be Thread::kNoDeoptId because below we assign NULL
environment to it anyway.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/constant_pro...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/constant_pro...
runtime/vm/constant_propagator.cc:1552: if_true->AsJoinEntry(),
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/constant_pro...
runtime/vm/constant_propagator.cc:1697: Thread::Current()->GetNextDeoptId());
ditto

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/constant_pro...
runtime/vm/constant_propagator.cc:1704: join = new (Z)
JoinEntryInstr(if_true->block_id(), if_true->try_index(),
ditto

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/constant_pro...
runtime/vm/constant_propagator.cc:1717: new (Z) GotoInstr(join,
Thread::Current()->GetNextDeoptId());
ditto

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

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:1022: new
CurrentContextInstr(Thread::Current()->GetNextDeoptId());
kNoDeoptId - it can't deopt anyway.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1636: owner()->GetNextDeoptId());  // Number
check.
comment is now misplaced. maybe fix it like this

/* number_check = */ true

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1519: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1522: new (Z) GotoInstr(new_join,
Thread::Current()->GetNextDeoptId());
ditto

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1663: Thread::Current()->GetNextDeoptId());
should be call_->deopt_id() I think because we use call->env() deopt environment
below

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1694: AllocateBlockId(), try_idx,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1766: new GotoInstr(join,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1797: new BranchInstr(compare_top,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1803: AllocateBlockId(), try_idx,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1814: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1818: new Value(load_cid), new
Value(cid_constant), false,
/* number_check = */ false 

because comment moved.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1819: Thread::Current()->GetNextDeoptId());  //
No number check.
kNoDeoptId because StrictCompareInstr never deopts.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1820: branch = new BranchInstr(compare,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1852: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1855: new GotoInstr(join,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1867: AllocateBlockId(), try_idx,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1878: AllocateBlockId(), try_idx,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1880: AllocateBlockId(), try_idx,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1885: new GotoInstr(join,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1887: new GotoInstr(join,
Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1923: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2309: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2359: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2497: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2522: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2545: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2570: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2657: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2701: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2868: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:2897: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3011: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3130: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3159: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3189: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3246: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3298: Thread::Current()->GetNextDeoptId());
kNoDeoptId because InheritDeoptTarget below.

same comment applies to all the code in this file.

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1342: LinkTo(new GotoInstr(entry,
Thread::Current()->GetNextDeoptId()));  // !!!!!
Stale comment?

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3813: Thread::Current()->GetNextDeoptId());
this is only called from optimizer so it should be kNoDeoptId (strict compare
never deopts).

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/jit_optimize...
File runtime/vm/jit_optimizer.cc (right):

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.cc:610: Thread::Current()->GetNextDeoptId());
kNoDeoptId (because we are in the optimizer and strict compare can't deopt)

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.cc:1366: new (Z) Value(cid), false,  // No number
check.
maybe better /* number_check = */ false

https://codereview.chromium.org/2896903002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.cc:1367: Thread::Current()->GetNextDeoptId());
ditto

[rmacnak, 2017-05-23 23:33:59]

Description was changed from

==========
Shuffle around deopt id allocation to give the flow graph builder a chance to
record other data as they are allocated.
==========

to

==========
Shuffle around deopt id allocation to give the flow graph builder a chance to
record other data as they are allocated.

We'll use deopt-id -> context-level or deopt-id -> scope mappings to find what
variables are in scope for the debugger and async stack trace machinery.
==========

[rmacnak, 2017-05-23 23:37:02]

Updated to using Thread::kNoDeoptId in the optimization passes and inliner.

Updated CreateArrayInstr to allow for a missing deopt id when optimizing.

Updated argument comments to /* C */ comments.

[rmacnak, 2017-05-23 23:56:19]

Description was changed from

==========
Shuffle around deopt id allocation to give the flow graph builder a chance to
record other data as they are allocated.

We'll use deopt-id -> context-level or deopt-id -> scope mappings to find what
variables are in scope for the debugger and async stack trace machinery.
==========

to

==========
Shuffle around deopt id allocation to give the flow graph builder a chance to
record other data as they are allocated.

We'll use deopt-id -> context-level or deopt-id -> scope mappings to find what
variables are in scope for the debugger and async stack trace machinery.

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c94bebf28937e5250b2d6486503848645be142b1
==========

[rmacnak, 2017-05-23 23:56:20]

Committed patchset #3 (id:40001) manually as
c94bebf28937e5250b2d6486503848645be142b1 (presubmit successful).