Make CTX allocatable by the register allocator.

Make CTX allocatable by the register allocator.

This change makes CTX available by not caching the current
context while in Dart code. Instead the current context
is held in a local variable (:saved_current_context_var) and
is passed as argument in CTX at calls.

This also simplifies a lot of code in the debugger: As a result,
Isolate::top_context is not needed anymore since the current context
can always be extracted from a Dart frame.

R=vegorov@google.com

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

[Florian Schneider, 2014-10-27 14:34:30]

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

[Florian Schneider, 2014-10-27 14:34:30]

Ready for review (ia32 only for now). I will start porting once we agreed on the
approach.

[Florian Schneider, 2014-10-28 12:13:11]

Simplified further so that there is no need to pass a context in a register at
calls. Closures load it from their first parameter, other functions use the
constant empty context.

[Vyacheslav Egorov (Google), 2014-10-28 13:44:58]

overall I think it is shaping good, some initial comments.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:471: if (store->local().index() ==
flow_graph_->CurrentContextVar()->index()) {
is it possible just to do

store->local().Equals(flow_graph_->CurrentContextVar())

instead?

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:489: if (load->local().index() ==
flow_graph_->CurrentContextVar()->index()) {
ditto

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:734: bool always_live = var_index ==
CurrentContextEnvIndex();
const bool is_always_alive

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:753: block->AsJoinEntry()->InsertPhi(var_index,
You can also make InsertPhi return the phi it created and then do mark_alive
here.

Also all live phis need to be added to live_phis list because alive mark is
propagated upwards to used phis.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:804: // Initialize all locals with #null in the
renaming environment.  For OSR,
Comment is out of date.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:894: if (!live_in->Contains(i) && (i !=
CurrentContextEnvIndex())) {
Should not we instead ensure that live_in->Contains(CurrentContextEnvIndex())
then we would not need to special case it here.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.h#...
runtime/vm/flow_graph.h:176: while (env->outer() != NULL) env = env->outer();
Kill this function. It's also a bit suspicious.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.h#...
runtime/vm/flow_graph.h:355: Definition* current_context_;
Kill this with fire.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:822:
*owner()->parsed_function()->saved_current_context_var(), value);
I would call it just current_context_var()

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1077: if (function.IsClosureFunction() &&
!flow_graph().IsCompiledForOsr()) {
It seems closure calling sequence now has a redundant load then. We load it
twice - once in a caller and once in a callee.

Why load in the caller to begin with?

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:935:
context->ReplaceUsesWith(caller_graph_->CurrentContext(call->env()));
Just use the empty one here. If it's not a closure call it does not need no
context.

Potentially this is even dead code because in the flow graph construction for
non-closures we already put empty context into this slot and not
CurrentContextInstr.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:1354:
owner_->caller_graph()->CurrentContext(call_data.call->env()));
Ditto.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/parser_test.c...
runtime/vm/parser_test.cc:280: " 2 SavedEntryCtx scope=0   begin=0   end=0"
do we still need SavedEntryCtx if we always save in the caller?

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:50: __ LoadIsolate(EAX);
if you were to load isolate into EAX into a callee-save register then there
would be no need to reload it after the native call below

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:254: __ LoadIsolate(EDI);
no need to reload, EDI and ESI are both callee save.

[Florian Schneider, 2014-10-28 19:04:31]

Addressed comments. I will start porting to the other platforms now.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:471: if (store->local().index() ==
flow_graph_->CurrentContextVar()->index()) {
On 2014/10/28 13:44:57, Vyacheslav Egorov (Google) wrote:
> is it possible just to do
> 
> store->local().Equals(flow_graph_->CurrentContextVar())
> 
> instead?

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:489: if (load->local().index() ==
flow_graph_->CurrentContextVar()->index()) {
On 2014/10/28 13:44:57, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:734: bool always_live = var_index ==
CurrentContextEnvIndex();
On 2014/10/28 13:44:57, Vyacheslav Egorov (Google) wrote:
> const bool is_always_alive

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:753: block->AsJoinEntry()->InsertPhi(var_index,
On 2014/10/28 13:44:57, Vyacheslav Egorov (Google) wrote:
> You can also make InsertPhi return the phi it created and then do mark_alive
> here.
> 
> Also all live phis need to be added to live_phis list because alive mark is
> propagated upwards to used phis. 

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:804: // Initialize all locals with #null in the
renaming environment.  For OSR,
On 2014/10/28 13:44:57, Vyacheslav Egorov (Google) wrote:
> Comment is out of date.

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:894: if (!live_in->Contains(i) && (i !=
CurrentContextEnvIndex())) {
On 2014/10/28 13:44:57, Vyacheslav Egorov (Google) wrote:
> Should not we instead ensure that live_in->Contains(CurrentContextEnvIndex())
> then we would not need to special case it here.

Yes, I can change the VariableLiveness accordingly. Added TODO for now. I simply
handled the context variable in all places where we replace dead variables with
the <optimized out> constant.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.h#...
runtime/vm/flow_graph.h:176: while (env->outer() != NULL) env = env->outer();
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> Kill this function. It's also a bit suspicious.

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph.h#...
runtime/vm/flow_graph.h:355: Definition* current_context_;
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> Kill this with fire.

Done.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:822:
*owner()->parsed_function()->saved_current_context_var(), value);
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> I would call it just current_context_var()

Done. I just used the name of the variable that was already there.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1077: if (function.IsClosureFunction() &&
!flow_graph().IsCompiledForOsr()) {
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> It seems closure calling sequence now has a redundant load then. We load it
> twice - once in a caller and once in a callee.
> 
> Why load in the caller to begin with?

Done. This was only done for now to make inlining simpler. I removed the load
from the calling sequence and added the necessary context load in the inliner.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:935:
context->ReplaceUsesWith(caller_graph_->CurrentContext(call->env()));
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> Just use the empty one here. If it's not a closure call it does not need no
> context.
> 
> Potentially this is even dead code because in the flow graph construction for
> non-closures we already put empty context into this slot and not
> CurrentContextInstr.

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:1354:
owner_->caller_graph()->CurrentContext(call_data.call->env()));
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/parser_test.c...
runtime/vm/parser_test.cc:280: " 2 SavedEntryCtx scope=0   begin=0   end=0"
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> do we still need SavedEntryCtx if we always save in the caller?

Yes, I'm pretty sure that this can be simplfied further. I'd rather not load too
much into this CL though.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/stub_code_ia3...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:50: __ LoadIsolate(EAX);
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> if you were to load isolate into EAX into a callee-save register then there
> would be no need to reload it after the native call below

Done.

https://codereview.chromium.org/678763004/diff/20001/runtime/vm/stub_code_ia3...
runtime/vm/stub_code_ia32.cc:254: __ LoadIsolate(EDI);
On 2014/10/28 13:44:58, Vyacheslav Egorov (Google) wrote:
> no need to reload, EDI and ESI are both callee save.

Done.

[Florian Schneider, 2014-10-29 15:14:33]

Ptal. Ported to all platforms.

[hausner, 2014-10-29 16:44:33]

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

[hausner, 2014-10-29 16:44:34]

DBCs

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

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:474: const Object& context =
Object::Handle(GetLocalVar(var_info.index()));
Would be nice if you could avoid the handle creation in the non-debug build. You
only need it for the assertion. Alternatively, you can assert that the class id
is kContextCid instead of calling IsContext().

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

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:130: LocalVariable*
ParsedFunction::EnsureSavedCurrentContext() {
Now that every function has a local variable to store the current context, the
variable can be set up proactively in the ParsedFunction constructor, rather
than calling this function from the parser constructor.

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:881: if (parsed_function->has_current_context_var()) {
This is always true now, correct?

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:1085: if (parsed_function->has_current_context_var()) {
ditto

[Florian Schneider, 2014-10-29 17:50:45]

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

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:474: const Object& context =
Object::Handle(GetLocalVar(var_info.index()));
On 2014/10/29 16:44:34, hausner wrote:
> Would be nice if you could avoid the handle creation in the non-debug build.
You
> only need it for the assertion. Alternatively, you can assert that the class
id
> is kContextCid instead of calling IsContext().

Done, but I still need a cast (the result of GetLocalVar to a RawContext*) if I
don't want to change all call-sites of GetSavedCurrentContext.

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

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:130: LocalVariable*
ParsedFunction::EnsureSavedCurrentContext() {
On 2014/10/29 16:44:34, hausner wrote:
> Now that every function has a local variable to store the current context, the
> variable can be set up proactively in the ParsedFunction constructor, rather
> than calling this function from the parser constructor.

Done. Yes, I can now remove this function.

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:881: if (parsed_function->has_current_context_var()) {
On 2014/10/29 16:44:34, hausner wrote:
> This is always true now, correct? 

Done.

https://codereview.chromium.org/678763004/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:1085: if (parsed_function->has_current_context_var()) {
On 2014/10/29 16:44:34, hausner wrote:
> ditto

Done.

[Vyacheslav Egorov (Google), 2014-10-30 15:05:35]

lgtm

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

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:475: return
reinterpret_cast<RawContext*>(GetLocalVar(var_info.index()));
We did not worry much in the previous code about Handle allocation so I don't
think we should resort to reinterpret_cast in this code either.

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1082: __ ldr(CTX, FieldAddress(CTX,
Closure::context_offset()));
Not all optimized closure functions would use context so it would be redundant
to load it. 

Add TODO to elide this load.

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_t...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_t...
runtime/vm/flow_graph_type_propagator.cc:553: if (cid_ == kFunctionCid || cid_
== kContextCid) {
Align with Koda's CL on this.

[Florian Schneider, 2014-10-30 15:24:32]

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

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:475: return
reinterpret_cast<RawContext*>(GetLocalVar(var_info.index()));
On 2014/10/30 15:05:35, Vyacheslav Egorov (Google) wrote:
> We did not worry much in the previous code about Handle allocation so I don't
> think we should resort to reinterpret_cast in this code either.

It's probably not performance critical here. I don't really care which way.

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1082: __ ldr(CTX, FieldAddress(CTX,
Closure::context_offset()));
On 2014/10/30 15:05:35, Vyacheslav Egorov (Google) wrote:
> Not all optimized closure functions would use context so it would be redundant
> to load it. 
> 
> Add TODO to elide this load.
> 

Done.

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_t...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_t...
runtime/vm/flow_graph_type_propagator.cc:553: if (cid_ == kFunctionCid || cid_
== kContextCid) {
On 2014/10/30 15:05:35, Vyacheslav Egorov (Google) wrote:
> Align with Koda's CL on this.

Done.

[Florian Schneider, 2014-10-30 15:43:18]

Committed patchset #7 (id:120001) manually as r41422 (presubmit successful).