Support reusable boxes for Float32x4 fields

Extend StoreInstanceFieldInstr and LoadFieldInstr to support reusable Float32x4 boxes on IA32, X64, and ARM.

R=fschneider@google.com

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

[srdjan, 2014-02-05 22:20:09]

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:462: intptr_t live_registers_size =
registers->CpuRegisterCount() +
const intptr_t

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:18: #include "vm/il_printer.h"
Please use alphabetic order.

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6466: valid_class;
How about moving Field::IsUnboxedField in an architecture dependent file?
intermediate_language_xxx.cc?

[Cutch, 2014-02-05 23:16:02]

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:462: intptr_t live_registers_size =
registers->CpuRegisterCount() +
On 2014/02/05 22:20:09, srdjan wrote:
> const intptr_t

Done.

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:18: #include "vm/il_printer.h"
On 2014/02/05 22:20:09, srdjan wrote:
> Please use alphabetic order.

Removed.

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/150063004/diff/310001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6466: valid_class;
On 2014/02/05 22:20:09, srdjan wrote:
> How about moving Field::IsUnboxedField in an architecture dependent file?
> intermediate_language_xxx.cc?

Done.

[Florian Schneider, 2014-02-06 12:38:48]

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:466: ASSERT(bitmap->Length() <= (StackSize() +
live_registers_size));
We should comment what's going on here:

Maybe add a comment saying that bitmap maybe larger if there are >1 safepoint at
an instruction.

An instruction may have two safepoints in deferred code (like in this CL). The
call to RecordSafepoint has the side-effect of appending the live registers to
the bitmap. This is why the second call to RecordSafepoint with the same
instruction (and same location summary) sees a bitmap that is larger than
StackSize().

Right now, the second RecordSafepoint then truncates bitmap again to StackSize
and appends the live-registers again. - It works but looks a little strange and
all safe points in one instruction have the same bitmap in the end.

I know I suggested to refactor the code to use only one slow-path code object.
But could you try your previous patch set with this updated assert? I think it
will work, and it may be more concise with two slow-path objects.

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1584: bool Field::IsUnboxedField() const
{
Maybe this should still be in object.cc? In this case I would rather use  

#ifdef ARCH_MIPS

since MIPS does not support Float32x4Cid. This way you can avoid duplicating the
code in the 4 different platform-specific files.

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1630:
locs->live_registers()->Remove(locs->out());
locs->live_registers is already modified in the same way before, so this line
does not change anything.

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1839:
locs()->live_registers()->Add(locs()->in(0));
locs() is already modified in the same way above, so these two lines do not
change anything.

https://codereview.chromium.org/150063004/diff/380001/tests/language/vm/reusa...
File tests/language/vm/reusable_boxes_test.dart (right):

https://codereview.chromium.org/150063004/diff/380001/tests/language/vm/reusa...
tests/language/vm/reusable_boxes_test.dart:36: f.test();
Maybe return a value to check from test(), or check that f.c contains the
correct value afterwards.

https://codereview.chromium.org/150063004/diff/380001/tests/language/vm/reusa...
tests/language/vm/reusable_boxes_test.dart:67: f.test();
Maybe return a value to check from test(), or check that f.c contains the
correct value afterwards.

[srdjan, 2014-02-06 16:55:31]

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1584: bool Field::IsUnboxedField() const
{
On 2014/02/06 12:38:49, Florian Schneider wrote:
> Maybe this should still be in object.cc? In this case I would rather use  
> 
> #ifdef ARCH_MIPS
> 
> since MIPS does not support Float32x4Cid. This way you can avoid duplicating
the
> code in the 4 different platform-specific files.

I do not like architecture dependent code in shared files. How about moving it
back to object.cc (I suggested moving them here, sorry) and add
SupportsFloat32x4 similar to SupportsUnboxedMints?

[Florian Schneider, 2014-02-06 16:57:17]

On 2014/02/06 16:55:31, srdjan wrote:
>
https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
> File runtime/vm/intermediate_language_arm.cc (right):
> 
>
https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
> runtime/vm/intermediate_language_arm.cc:1584: bool Field::IsUnboxedField()
const
> {
> On 2014/02/06 12:38:49, Florian Schneider wrote:
> > Maybe this should still be in object.cc? In this case I would rather use  
> > 
> > #ifdef ARCH_MIPS
> > 
> > since MIPS does not support Float32x4Cid. This way you can avoid duplicating
> the
> > code in the 4 different platform-specific files.
> 
> I do not like architecture dependent code in shared files. How about moving it
> back to object.cc (I suggested moving them here, sorry) and add
> SupportsFloat32x4 similar to SupportsUnboxedMints?

Yes, that sounds good.

[Cutch, 2014-02-06 23:15:38]

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:466: ASSERT(bitmap->Length() <= (StackSize() +
live_registers_size));
On 2014/02/06 12:38:49, Florian Schneider wrote:
> We should comment what's going on here:
> 
> Maybe add a comment saying that bitmap maybe larger if there are >1 safepoint
at
> an instruction.
> 
> An instruction may have two safepoints in deferred code (like in this CL). The
> call to RecordSafepoint has the side-effect of appending the live registers to
> the bitmap. This is why the second call to RecordSafepoint with the same
> instruction (and same location summary) sees a bitmap that is larger than
> StackSize().
> 
> Right now, the second RecordSafepoint then truncates bitmap again to StackSize
> and appends the live-registers again. - It works but looks a little strange
and
> all safe points in one instruction have the same bitmap in the end.
> 
> I know I suggested to refactor the code to use only one slow-path code object.
> But could you try your previous patch set with this updated assert? I think it
> will work, and it may be more concise with two slow-path objects.
> 

Done.

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1584: bool Field::IsUnboxedField() const
{
On 2014/02/06 12:38:49, Florian Schneider wrote:
> Maybe this should still be in object.cc? In this case I would rather use  
> 
> #ifdef ARCH_MIPS
> 
> since MIPS does not support Float32x4Cid. This way you can avoid duplicating
the
> code in the 4 different platform-specific files.

Done.

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1630:
locs->live_registers()->Remove(locs->out());
On 2014/02/06 12:38:49, Florian Schneider wrote:
> locs->live_registers is already modified in the same way before, so this line
> does not change anything.

Done.

https://codereview.chromium.org/150063004/diff/380001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1839:
locs()->live_registers()->Add(locs()->in(0));
On 2014/02/06 12:38:49, Florian Schneider wrote:
> locs() is already modified in the same way above, so these two lines do not
> change anything.

Done.

https://codereview.chromium.org/150063004/diff/380001/tests/language/vm/reusa...
File tests/language/vm/reusable_boxes_test.dart (right):

https://codereview.chromium.org/150063004/diff/380001/tests/language/vm/reusa...
tests/language/vm/reusable_boxes_test.dart:36: f.test();
On 2014/02/06 12:38:49, Florian Schneider wrote:
> Maybe return a value to check from test(), or check that f.c contains the
> correct value afterwards.

Done.

https://codereview.chromium.org/150063004/diff/380001/tests/language/vm/reusa...
tests/language/vm/reusable_boxes_test.dart:67: f.test();
On 2014/02/06 12:38:49, Florian Schneider wrote:
> Maybe return a value to check from test(), or check that f.c contains the
> correct value afterwards.

Done.

[Cutch, 2014-02-06 23:45:43]

Latest CL:

$ ./tools/test.py -a all --checked
--vm-options=--optimization_counter_threshold=5
Test configurations: none_vm_debug_ia32_checked none_vm_debug_x64_checked
none_vm_debug_simarm_checked none_vm_debug_simmips_checked
[30:37 | 100% | +45181 | -    0]

[Florian Schneider, 2014-02-07 13:30:00]

LGTM.

[Cutch, 2014-02-07 15:30:37]

Committed patchset #13 manually as r32429 (presubmit successful).