X64: Convert HeapNumbers that contain valid smi values to smis in binop-stub.

X64: Convert HeapNumbers that contain valid smi values to smis in binop-stub.

When the TypeRecordingBinaryOpStub expect smi values as input, they might
sometimes come as HeapNumbers. The transition code will detect the heap numbers
as holding values that are valid smi values, and will not change the expectations.
However, the stub didn't handle HeapNumbers and always tried to transition again.

Committed: http://code.google.com/p/v8/source/detail?r=7560

[William Hesse, 2011-04-08 09:27:58]

I think the change can be made smaller and simpler, see comments.

http://codereview.chromium.org/6812046/diff/1/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/6812046/diff/1/src/v8globals.h#newcode350
src/v8globals.h:350: explicit DoubleRepresentation(double x) { memcpy(&bits, &x,
sizeof(bits)); }
Leave as is, or use BitCast - we don't want yet a third way of doing this.

http://codereview.chromium.org/6812046/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6812046/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1168: __ SmiOr(right, right, left);  // BIT_OR is
commutative.
Change to SmiOrAndJumpIfNotBothSmi(right, right, left, rcx), and leave the
functionality as is. The second time you reach this (after LoadUnknownsAsSmis),
you know that it will succeed, so no infinite loop.

http://codereview.chromium.org/6812046/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1245: {  // See if there were smi values stored in
HeapNumbers.
I would use LoadAsIntegers (really LoadUnknownsAsIntegers), followed by
SmiTag(), for simplicity's sake.

If you really want to avoid the extra smi-tagging, then I would write the
LoadUnknownsAsSmis function, and call that.  But this code is not that fast
anyway - at most you could make a ToDo for this optimization.

[Lasse Reichstein, 2011-04-08 11:18:10]

http://codereview.chromium.org/6812046/diff/1/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/6812046/diff/1/src/v8globals.h#newcode350
src/v8globals.h:350: explicit DoubleRepresentation(double x) { memcpy(&bits, &x,
sizeof(bits)); }
agree. Removed.

http://codereview.chromium.org/6812046/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6812046/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:1245: {  // See if there were smi values stored in
HeapNumbers.
LoadNumberAsIntegers will do truncating conversion as ToInteger32 (it's used
before bit-ops). It doesn't check whether the value is a valid smi before
truncating.
We don't want that here - if the value isn't something that could be a smi, we
should fail and go to the type transition.
There is no bailout in the code that allows us to do this.

[William Hesse, 2011-04-08 12:01:27]

LGTM.  See comments.

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:471: // (If you just want to fall through in all
cases, put the success
Why let the user of this function make a mistake.  NULL means that all successes
fall through, non-NULL means that all successes go there.

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2102: __ LoadRoot(heap_number_root,
Heap::kHeapNumberMapRootIndex);
Shouldn't this be called heap_number_map?

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2124: // One of first or second should be non-smi if
we get here.
Second should be non-smi if we get here.

[Lasse Reichstein, 2011-04-08 12:07:46]

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:471: // (If you just want to fall through in all
cases, put the success
Done.

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2102: __ LoadRoot(heap_number_root,
Heap::kHeapNumberMapRootIndex);
Absolutely.

http://codereview.chromium.org/6812046/diff/6/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2124: // One of first or second should be non-smi if
we get here.
Done.