MIPS: Fix NaN value inconsistency with snapshots (alternate implementation).

MIPS: Fix NaN value inconsistency with snapshots (alternate implementation).

The NaN values from roots and global object NaN property are incorrect
for MIPS HW when snapshot generated on simulator is used. This due to
difference in qNaN encoding on MIPS and ia32 architectures.

This version uses Erik's suggestion to find and replace all NaN heap
objects at serialization time, when performance is not an issue.

This must patch the bits without using type 'double', or debug (-O0)
builds will quiet the ia32 sNaN (same value as mips qNaN), which then 
corrupts the bit pattern for mips.

BUG=
TEST=cctest/test-api/QuietSignalingNaNs, mjsunit/harmony/collections.js

[danno, 2012-04-03 09:14:42]

Can you just make OS::nan_value() return the right non-signaling qNaN for MIPS
when running with the simulator on ia32?

https://chromiumcodereview.appspot.com/9910029/diff/1/src/runtime.js
File src/runtime.js (right):

https://chromiumcodereview.appspot.com/9910029/diff/1/src/runtime.js#newcode50
src/runtime.js:50: var $NaN = getCanonicalNan();
Why the extra wrapped function? Why not call %getCannonicalNaN here?

[Erik Corry, 2012-04-03 11:40:55]

It would probably be fine just to fix void
Serializer::ObjectSerializer::Serialize() {
so that it checks whether it has a heap number that is a NaN and then writes the
correct NaN.  There should be no impact at runtime, only at mksnapshot time.

[Paul Lind, 2012-04-04 05:02:47]

On 2012/04/03 09:14:42, danno wrote:
> Can you just make OS::nan_value() return the right non-signaling qNaN for
> MIPS when running with the simulator on ia32?

That does not work, since the MIPS qNaN encoding looks like sNaN to ia32. Any FP
operation that uses that value then "quiets" the signaling NaN, which then is
incorrect for MIPS.

I've removed the un-needed wrapper function here, just in case this version is
needed.

But I will be uploading a new version shortly using Erik's suggestion.

https://chromiumcodereview.appspot.com/9910029/diff/1/src/runtime.js
File src/runtime.js (right):

https://chromiumcodereview.appspot.com/9910029/diff/1/src/runtime.js#newcode50
src/runtime.js:50: var $NaN = getCanonicalNan();
On 2012/04/03 09:14:43, danno wrote:
> Why the extra wrapped function? Why not call %getCannonicalNaN here?

Not needed, removed.

[Paul Lind, 2012-04-04 05:17:52]

On 2012/04/03 11:40:55, Erik Corry wrote:
> It would probably be fine just to fix void
> Serializer::ObjectSerializer::Serialize() {
> so that it checks whether it has a heap number that is a NaN and then writes
the
> correct NaN.  There should be no impact at runtime, only at mksnapshot time.

Thanks for the suggestion. This is what is implemented now, in patchset 3. PTAL.

[Paul Lind, 2012-04-05 04:12:25]

Updated patch to fix debug builds. When using doubles and
HeapNumber::set_value(), code build with -O0 would use
ia32 FPU ops, which quieted the signaling NaN, once again
causing test failure. Now the HeapNumber value is updated
directly using memcpy().

[Erik Corry, 2012-04-12 09:19:38]

I made what i think is a prettier version at
https://chromiumcodereview.appspot.com/10068006

[Erik Corry, 2012-04-12 09:21:49]

If the simulator does not stop on signalling NaNs then I think that it should.

[Paul Lind, 2012-04-13 04:56:33]

Erik - Your patch does look prettier, thanks.  Works fine for me on sim & hw,
but I see it fails on your sim builder. I suspect diff compiler revs may change
the FP code that is emitted (mine quiets that sNaN on sim. yours does not) My
sim builds are on Ubuntu 10.04/gcc-4.4.3 and on Mac 10.6.8-gcc-4.2.1. What are
you using on your buildbot?

[Erik Corry, 2012-04-13 12:16:10]

I presume you mean the MIPS simulator on
http://build.chromium.org/p/client.v8/console but the newest failures I can see
there do not look NaN related.  Do you have a link to the failures you mean?

[kalmard, 2012-04-13 12:31:26]

On 2012/04/13 12:16:10, Erik Corry wrote:
> I presume you mean the MIPS simulator on
> http://build.chromium.org/p/client.v8/console but the newest failures I can
see
> there do not look NaN related.  Do you have a link to the failures you mean?

Hi! The new failures are caused by a completely different issue (an ARM change,
I already submitted the MIPS port to code review).

The NaN issues only manifest in the mjsunit harmony-collection failure.

We have a proposed workaround that's under internal code review. The patch
checks for MIPS NaN values during deserialization on the simulator and converts
them back to x86 NaNs. Is this an acceptable solution for you or should we find
another approach?

[Paul Lind, 2012-04-13 14:50:05]

> On 2012/04/13 12:16:10, Erik Corry wrote:
> > I presume you mean the MIPS simulator on
> > http://build.chromium.org/p/client.v8/console but the newest failures I can
> see
> > there do not look NaN related.  Do you have a link to the failures you mean?

Here's a link to to the build for this change, 11283, which scrolled off console
view:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20mips%20-%20si...

Fails in  collections.js on your sim.

Again, unusual thing is that my Sim does NOT fail. Looking into root cause, but
if it is something like compiler version, then this approach would seem fragile.

Daniel K.'s fix he mentioned is undoing the ia32 damage on the Sim at
de-serialization time, but then we are touching this twice, and may be better to
just do it once, at de-ser per Patchset 2 above.

[Paul Lind, 2012-04-14 03:38:10]

I had a bug in my tester config for Linux. It does fail for me now. (however, it
does pass on Mac). I'll upload a new patch sometime before Monday for your
review.

[Paul Lind, 2012-04-16 04:42:30]

After looking more, I think that trying to patch ia32 sNaN's into the heap
exposes fragile behavior, including compiler version dependencies. Seems much
cleaner to do this on the end target after de-ser.

New CL: https://chromiumcodereview.appspot.com/10093007/, properly tested this
time.

If you feel strongly otherwise I have another (un-published) CL here:
https://chromiumcodereview.appspot.com/10093008, but I am pretty uncomfortable
with this one, as it's a workaround for a bad fix, IMO. But it does work.