Refactor building of StoreStaticField and StoreLocal to manually preserve value.

Refactor building of StoreStaticField and StoreLocal to manually preserve value.

This is first step towards eliminating manual value preservation in the write barrier's fast-path.

R=fschneider@google.com
BUG=

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

[Florian Schneider, 2012-09-10 14:38:21]

LGTM with comments.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:676: kResultNotNeeded));
The parameter result_not_needed is not needed when storing to expr_temp since it
is never captured.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:1897: Value* allocated_value =
Bind(store_allocated);
Maybe rephrase this for consistency:

Value* allocated_value = Bind(BuildStoreLocal(node->allocated_object_var(),
allocate, kResultNeeded);

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:2115: void
EffectGraphVisitor::VisitStoreLocal(StoreLocalNode* node,
Maybe HandleStoreLocal, otherwise it looks like a visitor function.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:2205:
BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),
I'd have a separate helper to avoid confusion:

 BuildStoreTempLocal(const LocalVariable& temp, Value* value)  and BuildLoadTemp
for storing/loading into temporary locals (they are never captured, and
therefore don't need the result_is_needed parameter).

[Vyacheslav Egorov (Google), 2012-09-10 16:00:14]

thank you for the review.

comments addressed, landing.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:676: kResultNotNeeded));
On 2012/09/10 14:38:21, Florian Schneider wrote:
> The parameter result_not_needed is not needed when storing to expr_temp since
it
> is never captured.

Done.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:1897: Value* allocated_value =
Bind(store_allocated);
On 2012/09/10 14:38:21, Florian Schneider wrote:
> Maybe rephrase this for consistency:
> 
> Value* allocated_value = Bind(BuildStoreLocal(node->allocated_object_var(),
> allocate, kResultNeeded);

Done.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:2115: void
EffectGraphVisitor::VisitStoreLocal(StoreLocalNode* node,
On 2012/09/10 14:38:21, Florian Schneider wrote:
> Maybe HandleStoreLocal, otherwise it looks like a visitor function.

Done.

https://chromiumcodereview.appspot.com/10905182/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:2205:
BuildStoreLocal(*owner()->parsed_function().expression_temp_var(),
On 2012/09/10 14:38:21, Florian Schneider wrote:
> I'd have a separate helper to avoid confusion:
> 
>  BuildStoreTempLocal(const LocalVariable& temp, Value* value)  and
BuildLoadTemp
> for storing/loading into temporary locals (they are never captured, and
> therefore don't need the result_is_needed parameter).

Done.