Allocation sinking for contexts.

Allocation sinking for contexts.

Improved aliasing computation in presence of Redefinition and AssertAssignable.

Added possibility for inlining annotations via --enable-inlining-annotations flag.

R=vegorov@google.com

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

[Florian Schneider, 2014-10-31 16:05:32]

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

[Florian Schneider, 2014-11-03 13:05:17]

New patchset that works without repeated load optimization.

[Vyacheslav Egorov (Google), 2014-11-03 13:37:49]

needs tests

https://codereview.chromium.org/184523002/diff/100001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/184523002/diff/100001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:1246:
AddConstant(Smi::Handle(Smi::New(mat->num_variables())), dest_index++);
Can you avoid writing it if (mat->cls() != context_class())

https://codereview.chromium.org/184523002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/184523002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5919: if (!HasLoadsFromPlace(instance,
&store_place)) {
HasLoadsFromPlace has to account for redifinitions as well. The same for
MarkStoredValuesEscaping.

There is also another place that traverses input_use_list in ComputeInitialSets.
There it's not a correctness issue, but a completeness issue.

(please add a test for correctness reasons).

[Florian Schneider, 2014-11-04 13:28:57]

https://codereview.chromium.org/184523002/diff/100001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/184523002/diff/100001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:1246:
AddConstant(Smi::Handle(Smi::New(mat->num_variables())), dest_index++);
On 2014/11/03 13:37:48, Vyacheslav Egorov (Google) wrote:
> Can you avoid writing it if (mat->cls() != context_class())

I could - I'm not sure if it is worth making the DeferredObject description
variable-length depending on the class.

[Florian Schneider, 2014-11-12 17:14:08]

PTAL. Added tests and inlining annotation to make the tests more reliable.

[Vyacheslav Egorov (Google), 2014-11-13 13:01:42]

LGTM!

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

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:618: HasAnnotation(function, "NeverInline")) {
Maybe make a Symbol for this?

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1716: HasAnnotation(function, "AlwaysInline"))
{
Maybe make a symbol for this?

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5924: bool HasLoadsFromPlace(Definition*
defn, const Place* place) {
It would be good if we could assert somehow that *top-level* invocation of
HasLoadsFromPlace(defn, place) is always happening for defn that *is*
allocation.

The same for AnyUseCreatesAlias and MarkStoredValuesEscaping.

https://codereview.chromium.org/184523002/diff/160001/tests/language/vm/load_...
File tests/language/vm/load_elimination_has_loads_from_place_test.dart (right):

https://codereview.chromium.org/184523002/diff/160001/tests/language/vm/load_...
tests/language/vm/load_elimination_has_loads_from_place_test.dart:27: return
t.bval;
I would be also good to test the case when you store on one Redefinition and you
load it from another one (you need to inline twice).

[Florian Schneider, 2014-11-13 13:38:53]

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

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:618: HasAnnotation(function, "NeverInline")) {
On 2014/11/13 13:01:42, Vyacheslav Egorov (Google) wrote:
> Maybe make a Symbol for this?

I'll make it a constant variable. Symbol is not needed since it is only a const
char* - and used only for comparison.

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1716: HasAnnotation(function, "AlwaysInline"))
{
On 2014/11/13 13:01:42, Vyacheslav Egorov (Google) wrote:
> Maybe make a symbol for this?

Done. Same as above.

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/184523002/diff/160001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5924: bool HasLoadsFromPlace(Definition*
defn, const Place* place) {
On 2014/11/13 13:01:42, Vyacheslav Egorov (Google) wrote:
> It would be good if we could assert somehow that *top-level* invocation of
> HasLoadsFromPlace(defn, place) is always happening for defn that *is*
> allocation.
> 
> The same for AnyUseCreatesAlias and MarkStoredValuesEscaping.

Done.

https://codereview.chromium.org/184523002/diff/160001/tests/language/vm/load_...
File tests/language/vm/load_elimination_has_loads_from_place_test.dart (right):

https://codereview.chromium.org/184523002/diff/160001/tests/language/vm/load_...
tests/language/vm/load_elimination_has_loads_from_place_test.dart:27: return
t.bval;
On 2014/11/13 13:01:42, Vyacheslav Egorov (Google) wrote:
> I would be also good to test the case when you store on one Redefinition and
you
> load it from another one (you need to inline twice).

Done. Added a new test with two polymorphic calls so that the same allocations
gets redefined twice.

[Florian Schneider, 2014-11-13 13:54:20]

Committed patchset #11 (id:200001) manually as r41713 (presubmit successful).

[Ivan Posva, 2014-11-21 06:21:10]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2014-11-21 06:21:10]

-ivan

https://codereview.chromium.org/184523002/diff/200001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/184523002/diff/200001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:770: Object& value = Instance::Handle();
?