Add mutable double boxes for fields.

Add mutable double boxes for fields.

This allows the optimizing compiler to generate unboxed loads/stores
to fields containing double values. The double value is stored
in a reusable double object.

Unboxed loads/stores are generated for optimized code. Unoptimized code
allocates a new double on loads. To avoid performance regressions
for fields that are only written few times (e.g. only in the constructor)
I put a heuristic in place that
compares the usage count of setters and getters. Unboxed operations
are only generated if the setter is invoked a significant amount of
times (threshold is 10% of getter invocations).

The CL is so big because it changes the way LocationSummmary
is allocated: We now have a bit to generate different summaries
for optimized and unoptimized code.

R=srdjan@google.com

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

[srdjan, 2013-12-13 18:13:16]

Adding Zach for Mips/Arm changes.

"APP"  means "Add parentheses please".

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler.cc:640: if (load_node.field().guarded_cid() ==
kDynamicCid) {
Please add comment why.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:3524: && setter.usage_counter() > 0
Add parentheses here and below.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:143: && field()->IsUnboxedField();
APP

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3461: // May require a deoptimziation target
for input conversions.
s/deoptimziation/deoptimization/

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3463: return deopt_id_;
return GetDeoptId() (similar to other places).

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3478: ASSERT(index == 0 || index == 1);
Add parentheses please. Introducing an abbreviation for that: APP :-).

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3479: if (index == 1 && IsUnboxedStore())
return kUnboxedDouble;
APP

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3496: 
Please add brief comment what initialization_ means. Personally, I think Boolean
variables are more readable if they are prefixed with is_ or has_. In this case
is_initialization_.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:139:
comparison()->InitializeLocationSummary(optimizing);
I guess this does no compile ... optimizing not defined. I would replace
everywhere he opt with optimizing. Though I think it is more readable
is_optimizing. If length is a concern , maybe is_opt?

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:1618: if (IsUnboxedStore() &&
optimizing) {
ditto for optimizing

https://codereview.chromium.org/99573005/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/object.h#newcode2148
runtime/vm/object.h:2148: && guarded_cid() == kDoubleCid && !is_nullable();
APP

https://codereview.chromium.org/99573005/diff/1/runtime/vm/object.h#newcode2154
runtime/vm/object.h:2154: (!is_final() && guarded_cid() == kIllegalCid));
APP

[Cutch, 2013-12-13 19:48:43]

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:4192: return IsUnboxedLoad() ? kUnboxedDouble
: kTagged;
Can we not hardcode kUnboxedDouble as the representation of unboxed loads and
stores? It would be better if the representation was based on the guarded field
type.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:1653: __
TryAllocate(compiler->double_class(),
Please allow the type of to be parameterized.

[zra, 2013-12-13 21:06:30]

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_mips.cc:138:
comparison()->InitializeLocationSummary(optimizing);
Same problem as ARM?

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_mips.cc:1686: if (IsUnboxedStore() &&
optimizing) {
optimizing

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_mips.cc:3730:
comparison()->InitializeLocationSummary(optimizing);
optimizing

[srdjan, 2013-12-13 22:12:05]

LGTM

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:3521:
Function::Handle(owner.LookupSetterFunction(field_name));
Below is an important heuristic, add brief comment.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:3525: && 10 * setter.usage_counter() >
getter.usage_counter();
put 10 into a flag to experiment with ?

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:1599:
locs()->live_registers()->Add(locs()->in(1));
This is for saving and restoring registers (aka slow path), isn't it?

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:1816: __ movq(temp,
FieldAddress(instance_reg, offset_in_bytes()));
I wonder if we could split the instructions, one to fetch the double object, the
other to load/store a double from/into it. (not in this CL of course).

[Florian Schneider, 2013-12-16 13:08:09]

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler.cc:640: if (load_node.field().guarded_cid() ==
kDynamicCid) {
On 2013/12/13 18:13:17, srdjan wrote:
> Please add comment why.

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:3521:
Function::Handle(owner.LookupSetterFunction(field_name));
On 2013/12/13 22:12:06, srdjan wrote:
> Below is an important heuristic, add brief comment.

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:3524: && setter.usage_counter() > 0
On 2013/12/13 18:13:17, srdjan wrote:
> Add parentheses here and below.

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:3525: && 10 * setter.usage_counter() >
getter.usage_counter();
On 2013/12/13 22:12:06, srdjan wrote:
> put 10 into a flag to experiment with ?

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:143: && field()->IsUnboxedField();
On 2013/12/13 18:13:17, srdjan wrote:
> APP

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3461: // May require a deoptimziation target
for input conversions.
On 2013/12/13 18:13:17, srdjan wrote:
> s/deoptimziation/deoptimization/

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3463: return deopt_id_;
On 2013/12/13 18:13:17, srdjan wrote:
> return GetDeoptId() (similar to other places).

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3478: ASSERT(index == 0 || index == 1);
On 2013/12/13 18:13:17, srdjan wrote:
> Add parentheses please. Introducing an abbreviation for that: APP :-).

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3479: if (index == 1 && IsUnboxedStore())
return kUnboxedDouble;
On 2013/12/13 18:13:17, srdjan wrote:
> APP

Again? :)

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3496: 
On 2013/12/13 18:13:17, srdjan wrote:
> Please add brief comment what initialization_ means. Personally, I think
Boolean
> variables are more readable if they are prefixed with is_ or has_. In this
case
> is_initialization_.

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:4192: return IsUnboxedLoad() ? kUnboxedDouble
: kTagged;
On 2013/12/13 19:48:43, Cutch wrote:
> Can we not hardcode kUnboxedDouble as the representation of unboxed loads and
> stores? It would be better if the representation was based on the guarded
field
> type.

Yes. I agree. I'd like to generalize the approach in a separate CL to avoid to
much complication for now.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:139:
comparison()->InitializeLocationSummary(optimizing);
On 2013/12/13 18:13:17, srdjan wrote:
> I guess this does no compile ... optimizing not defined. I would replace
> everywhere he opt with optimizing. Though I think it is more readable
> is_optimizing. If length is a concern , maybe is_opt?

OOps. Yes, I shortened it last minute to avoid breaking 300 lines because to too
many characters. For that reason I'll keep the short version for now.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:1618: if (IsUnboxedStore() &&
optimizing) {
On 2013/12/13 18:13:17, srdjan wrote:
> ditto for optimizing

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:1653: __
TryAllocate(compiler->double_class(),
On 2013/12/13 19:48:43, Cutch wrote:
> Please allow the type of to be parameterized. 

I'd rather generalize the approach in a separate CL.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_mips.cc:138:
comparison()->InitializeLocationSummary(optimizing);
On 2013/12/13 21:06:31, zra wrote:
> Same problem as ARM?

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_mips.cc:1686: if (IsUnboxedStore() &&
optimizing) {
On 2013/12/13 21:06:31, zra wrote:
> optimizing

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_mips.cc:3730:
comparison()->InitializeLocationSummary(optimizing);
On 2013/12/13 21:06:31, zra wrote:
> optimizing

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:1599:
locs()->live_registers()->Add(locs()->in(1));
On 2013/12/13 22:12:06, srdjan wrote:
> This is for saving and restoring registers (aka slow path), isn't it?

Yes, the optimizing compiler computes live_registers automatically. This is for
slow-path code in unoptimized code.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_x64.cc:1816: __ movq(temp,
FieldAddress(instance_reg, offset_in_bytes()));
On 2013/12/13 22:12:06, srdjan wrote:
> I wonder if we could split the instructions, one to fetch the double object,
the
> other to load/store a double from/into it. (not in this CL of course).

Possibly yes. The current approach makes the Load/Store quite complicated. It
would be nice to split it.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/99573005/diff/1/runtime/vm/object.h#newcode2148
runtime/vm/object.h:2148: && guarded_cid() == kDoubleCid && !is_nullable();
On 2013/12/13 18:13:17, srdjan wrote:
> APP

Done.

https://codereview.chromium.org/99573005/diff/1/runtime/vm/object.h#newcode2154
runtime/vm/object.h:2154: (!is_final() && guarded_cid() == kIllegalCid));
On 2013/12/13 18:13:17, srdjan wrote:
> APP

Done.

[Florian Schneider, 2013-12-16 15:11:47]

Committed patchset #3 manually as r31164 (presubmit successful).