Merge ConstantInstr -> UnboxDouble to UnboxedConstant. Reduces register usage and allows for variou…

Merge ConstantInstr -> UnboxDouble to UnboxedConstant. Reduces register usage and allows for various optimization with constants.

R=fschneider@google.com

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

[zra, 2014-05-01 18:34:14]

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:47: 10.0, 20.0, 30.0, 64.0, 255.0};
PI? e?

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:60: 
Missing \n

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1640: DRegister dst =
EvenDRegisterOf(destination.fpu_reg());
const DRegister dst

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:1641: __ LoadDImmediate(dst,
constant.value(), TMP);
Can't use TMP here.

[Florian Schneider, 2014-05-01 18:34:15]

lgtm

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:1610: Definition*
UnboxDoubleInstr::Canonicalize(FlowGraph* flow_graph) {
Maybe add

if (!HasUses()) return NULL;

so that Unbox operations that are not used anymore after canonicalization get
removed in a following pass.

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:1620: flow_graph->InsertBefore(this, uc,
this->env(), Definition::kValue);
UnboxedConstant should not need an environment, since it can't deoptimize and
can't become a deoptimization target. I'd pass NULL instead of this->env().

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:1621: return uc;
Make sure that the UnboxDoubleInstr get removed by the optimizer after the new
UnboxedConstant is inserted. It should not have any uses after this Canonicalize
step, so it can be just removed from the graph.

[Florian Schneider, 2014-05-01 18:36:23]

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:248: __ movsd(result,
FieldAddress(boxed, Double::value_offset()));
Do the same as on ia32 here?

[srdjan, 2014-05-01 20:33:03]

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:47: 10.0, 20.0, 30.0, 64.0, 255.0};
On 2014/05/01 18:34:14, zra wrote:
> PI? e

Added all math constant and NaN as well.

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:60: 
On 2014/05/01 18:34:14, zra wrote:
> Missing \n

Done.

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/266633007/diff/13/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1640: DRegister dst =
EvenDRegisterOf(destination.fpu_reg());
On 2014/05/01 18:34:14, zra wrote:
> const DRegister dst

Done.

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:1610: Definition*
UnboxDoubleInstr::Canonicalize(FlowGraph* flow_graph) {
On 2014/05/01 18:34:16, Florian Schneider wrote:
> Maybe add
> 
> if (!HasUses()) return NULL;
> 
> so that Unbox operations that are not used anymore after canonicalization get
> removed in a following pass.

Done.

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:1620: flow_graph->InsertBefore(this, uc,
this->env(), Definition::kValue);
On 2014/05/01 18:34:16, Florian Schneider wrote:
> UnboxedConstant should not need an environment, since it can't deoptimize and
> can't become a deoptimization target. I'd pass NULL instead of this->env().

Done.

https://codereview.chromium.org/266633007/diff/13/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:1621: return uc;
On 2014/05/01 18:34:16, Florian Schneider wrote:
> Make sure that the UnboxDoubleInstr get removed by the optimizer after the new
> UnboxedConstant is inserted. It should not have any uses after this
Canonicalize
> step, so it can be just removed from the graph.

I checked it visually in disassembled output/flow graph. Any other way?

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:1641: __ LoadDImmediate(dst,
constant.value(), TMP);
On 2014/05/01 18:34:14, zra wrote:
> Can't use TMP here.

Fixed:
      const DRegister dst = EvenDRegisterOf(destination.fpu_reg());
      __ LoadObject(TMP, constant);
      __ AddImmediate(TMP, TMP, Double::value_offset() - kHeapObjectTag);
      __ vldrd(dst, Address(TMP, 0));

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/266633007/diff/50001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:248: __ movsd(result,
FieldAddress(boxed, Double::value_offset()));
On 2014/05/01 18:36:23, Florian Schneider wrote:
> Do the same as on ia32 here?

Implemented LoadDoubleConstant, using it here.

[zra, 2014-05-01 20:41:19]

https://codereview.chromium.org/266633007/diff/70001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/266633007/diff/70001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2571: const int32_t offset =
FindImmediate(*reinterpret_cast<int64_t*>(&value));
See checks in CanLoadImmediateFromPool and the fixup for the low bit in
LaodImmediate. I think the same are needed here.

[srdjan, 2014-05-01 21:14:13]

https://codereview.chromium.org/266633007/diff/70001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/266633007/diff/70001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2571: const int32_t offset =
FindImmediate(*reinterpret_cast<int64_t*>(&value));
On 2014/05/01 20:41:19, zra wrote:
> See checks in CanLoadImmediateFromPool and the fixup for the low bit in
> LaodImmediate. I think the same are needed here.

Yes, removed, on ARM do it as previous without using the address.

[srdjan, 2014-05-01 22:03:30]

Committed patchset #7 manually as r35663 (presubmit successful).