MIPS: avoid alignment issues in FillHeapNumberWithRandom.

MIPS: avoid alignment issues in FillHeapNumberWithRandom.

Copy the value using memcpy on native MIPS targets to avoid unaligned memory access.

BUG=
TEST=cctest test-random/CrankshaftRandom

[Yang, 2012-03-06 17:38:38]

Adding Erik for a second opinion.

http://codereview.chromium.org/9617011/diff/1/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/9617011/diff/1/src/v8.cc#newcode227
src/v8.cc:227: #ifdef V8_HOST_ARCH_MIPS
We have a flag called V8_TARGET_CAN_READ_UNALIGNED in globals.h.  Maybe it's
better to check against this?

[Yang, 2012-03-06 17:40:36]

On 2012/03/06 17:38:38, Yang wrote:
> Adding Erik for a second opinion.
> 
> http://codereview.chromium.org/9617011/diff/1/src/v8.cc
> File src/v8.cc (right):
> 
> http://codereview.chromium.org/9617011/diff/1/src/v8.cc#newcode227
> src/v8.cc:227: #ifdef V8_HOST_ARCH_MIPS
> We have a flag called V8_TARGET_CAN_READ_UNALIGNED in globals.h.  Maybe it's
> better to check against this?

Just for clarification: this patch is to improve performance, V8 on MIPS would
still work without it?

[Erik Corry, 2012-03-07 08:25:53]

http://codereview.chromium.org/9617011/diff/1/src/v8.cc
File src/v8.cc (right):

http://codereview.chromium.org/9617011/diff/1/src/v8.cc#newcode227
src/v8.cc:227: #ifdef V8_HOST_ARCH_MIPS
On 2012/03/06 17:38:38, Yang wrote:
> We have a flag called V8_TARGET_CAN_READ_UNALIGNED in globals.h.  Maybe it's
> better to check against this?

I agree with this.

[Yang, 2012-03-07 09:01:57]

On 2012/03/07 08:25:53, Erik Corry wrote:
> http://codereview.chromium.org/9617011/diff/1/src/v8.cc
> File src/v8.cc (right):
> 
> http://codereview.chromium.org/9617011/diff/1/src/v8.cc#newcode227
> src/v8.cc:227: #ifdef V8_HOST_ARCH_MIPS
> On 2012/03/06 17:38:38, Yang wrote:
> > We have a flag called V8_TARGET_CAN_READ_UNALIGNED in globals.h.  Maybe it's
> > better to check against this?
> 
> I agree with this.

I suggest this as alternative: http://codereview.chromium.org/9592047/

[kalmard, 2012-03-08 09:33:16]

Thanks for the review.
Yang's patch at http://codereview.chromium.org/9592047/ works, with one minor
change:

HeapNumber::cast(heap_number)->set_value(num.double_value);

(the field needs to be identified explicitly).

Also I just realized that the first memcpy is unnecessary for this patch. I will
make the required changes in case you decide to go with this one.

[kalmard, 2012-03-08 09:37:03]

On 2012/03/06 17:40:36, Yang wrote:
> Just for clarification: this patch is to improve performance, V8 on MIPS would
> still work without it?

This is not performance-related, any unaligned access causes a SIGBUS (bus
error) on MIPS.

[Yang, 2012-03-08 10:37:49]

On 2012/03/08 09:37:03, kalmard wrote:
> On 2012/03/06 17:40:36, Yang wrote:
> > Just for clarification: this patch is to improve performance, V8 on MIPS
would
> > still work without it?
> 
> This is not performance-related, any unaligned access causes a SIGBUS (bus
> error) on MIPS.

Thanks, I update my CL at http://codereview.chromium.org/9592047/

Shouldn't the compiler be able to hide the complexity for aligned and unaligned
stores on MIPS?

[kalmard, 2012-03-09 09:30:59]

On 2012/03/08 10:37:49, Yang wrote:
> Thanks, I update my CL at http://codereview.chromium.org/9592047/

Thank you. Just to clarify, we are happy with your change and would prefer that
patch over this one.
 
> Shouldn't the compiler be able to hide the complexity for aligned and
unaligned
> stores on MIPS?

The MIPS ABI requires doubles and objects containing doubles to be aligned to 8
byte boundaries. This way the compiler can just assume that memory is properly
aligned when writing to a double. In this specific case this assumption is
wrong, since HeapNumber is not 8-byte aligned.

[Yang, 2012-03-09 09:45:28]

On 2012/03/09 09:30:59, kalmard wrote:
> On 2012/03/08 10:37:49, Yang wrote:
> > Thanks, I update my CL at http://codereview.chromium.org/9592047/
> 
> Thank you. Just to clarify, we are happy with your change and would prefer
that
> patch over this one.
>  
> > Shouldn't the compiler be able to hide the complexity for aligned and
> unaligned
> > stores on MIPS?
> 
> The MIPS ABI requires doubles and objects containing doubles to be aligned to
8
> byte boundaries. This way the compiler can just assume that memory is properly
> aligned when writing to a double. In this specific case this assumption is
> wrong, since HeapNumber is not 8-byte aligned.

HeapNumber::set_value inlines write_double_field (objects-inl.h), which has a
specific implementation for MIPS. Wouldn't that be sufficient?

[Yang, 2012-03-09 09:56:57]

On 2012/03/09 09:45:28, Yang wrote:
> On 2012/03/09 09:30:59, kalmard wrote:
> > On 2012/03/08 10:37:49, Yang wrote:
> > > Thanks, I update my CL at http://codereview.chromium.org/9592047/
> > 
> > Thank you. Just to clarify, we are happy with your change and would prefer
> that
> > patch over this one.
> >  
> > > Shouldn't the compiler be able to hide the complexity for aligned and
> > unaligned
> > > stores on MIPS?
> > 
> > The MIPS ABI requires doubles and objects containing doubles to be aligned
to
> 8
> > byte boundaries. This way the compiler can just assume that memory is
properly
> > aligned when writing to a double. In this specific case this assumption is
> > wrong, since HeapNumber is not 8-byte aligned.
> 
> HeapNumber::set_value inlines write_double_field (objects-inl.h), which has a
> specific implementation for MIPS. Wouldn't that be sufficient?

I just landed http://codereview.chromium.org/9592047/. Since we use
HeapNumber::set_value to write the double to memory, I guess this would make any
additional MIPS-specific patch unnecessary?

[kalmard, 2012-03-09 10:04:35]

On 2012/03/09 09:56:57, Yang wrote:
> > HeapNumber::set_value inlines write_double_field (objects-inl.h), which has
a
> > specific implementation for MIPS. Wouldn't that be sufficient?
> 
> I just landed http://codereview.chromium.org/9592047/. Since we use
> HeapNumber::set_value to write the double to memory, I guess this would make
any
> additional MIPS-specific patch unnecessary?

Yes, that patch works fine on MIPS by itself. Thanks again.