X64: Load HeapNumberMap once in BinaryOpStub, and reuse it.

X64: Load HeapNumberMap once in BinaryOpStub, and reuse it.

[Erik Corry, 2010-06-18 13:40:33]

LGTM, but there are clearly more opportunities here.

http://codereview.chromium.org/2819012/diff/1/2
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2819012/diff/1/2#newcode10545
src/x64/codegen-x64.cc:10545: GenerateTypeTransition(masm);
Is it safe to assume that r9 is unchanged here?  GenerateTypeTransition can't
cause a GC or overwrite r9?

http://codereview.chromium.org/2819012/diff/1/2#newcode10551
src/x64/codegen-x64.cc:10551: if (static_operands_type_.IsNumber() &&
FLAG_debug_code) {
What's with the indentation here?

http://codereview.chromium.org/2819012/diff/1/2#newcode10567
src/x64/codegen-x64.cc:10567: FloatingPointHelper::LoadFloatOperands(masm, xmm4,
xmm5);
This looks bad, but perhaps Bill is working on this.  First we check that it's a
number then we load the number into the XMM registers.  This duplicates the Smi
checks.  I think the best solution is that the new CheckNumberOperands should
take two optional arguments (default to no_reg) that say where the number should
go.  Or vice versa, the Loader should take an optional label pointer that tells
it to check.

http://codereview.chromium.org/2819012/diff/1/2#newcode10589
src/x64/codegen-x64.cc:10589: // Allocate heap number in new space.
The ARM port is smarter here.  On both the Intel ports we are testing for
sminess before allocating the result.  On ARM we allocate space at the point
where we load the Smi into the fp register, so we don't have to test again here.

http://codereview.chromium.org/2819012/diff/1/2#newcode10659
src/x64/codegen-x64.cc:10659: FloatingPointHelper::LoadAsIntegers(masm,
&call_runtime);
This should be passed the result of static_operands_type_.IsNumber() and the
heap number register.  It's doing unnecessary checking and it is loading the map
again.

http://codereview.chromium.org/2819012/diff/1/2#newcode10682
src/x64/codegen-x64.cc:10682: if (op_ == Token::SHR) {
How did the old version even work?

[Lasse Reichstein, 2010-06-21 07:51:37]

http://codereview.chromium.org/2819012/diff/1/2
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/2819012/diff/1/2#newcode10545
src/x64/codegen-x64.cc:10545: GenerateTypeTransition(masm);
Ick, I do believe it might make a runtime call (which will preserve r9, but may
cause a GC). I'll move initialization of r9 to after this.

http://codereview.chromium.org/2819012/diff/1/2#newcode10551
src/x64/codegen-x64.cc:10551: if (static_operands_type_.IsNumber() &&
FLAG_debug_code) {
Fixed.

http://codereview.chromium.org/2819012/diff/1/2#newcode10567
src/x64/codegen-x64.cc:10567: FloatingPointHelper::LoadFloatOperands(masm, xmm4,
xmm5);
I think Bill is working on this.

http://codereview.chromium.org/2819012/diff/1/2#newcode10589
src/x64/codegen-x64.cc:10589: // Allocate heap number in new space.
Added TODO to that effect. 
It should be done in the same rewrite as the "load float" changes.

http://codereview.chromium.org/2819012/diff/1/2#newcode10659
src/x64/codegen-x64.cc:10659: FloatingPointHelper::LoadAsIntegers(masm,
&call_runtime);
Avoided loading the map again. Leaving other changes for rewrite of
LoadAsInteger.

http://codereview.chromium.org/2819012/diff/1/2#newcode10682
src/x64/codegen-x64.cc:10682: if (op_ == Token::SHR) {
On 2010/06/18 13:40:34, Erik Corry wrote:
> How did the old version even work?

Easily. It went to slowcase on non-smi result of SHR, and the allocation code
was dead, since the non_smi_result label was only linked if op_ == Token::SHR.
I.e., it was a piece of crap.