LGTM w/ fix.
http://codereview.chromium.org/2862004/diff/4001/5001 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/2862004/diff/4001/5001#newcode9940 src/x64/codegen-x64.cc:9940: __ jmp(&load_rax); Maybe convert this jmp to JumpIfNotSmi(rax, &load_rax_heapnumber), to utilize the fallthrough. But then we might as well duplicate the load of rax's value, it's just one instruction. It's slightly larger, but looks like it should have better "flow". Do we sometimes know that not both can be smis? http://codereview.chromium.org/2862004/diff/4001/5001#newcode9962 src/x64/codegen-x64.cc:9962: __ cmpq(FieldOperand(rax, HeapObject::kMapOffset), rcx); rcx might not have been loaded here if rdx was a Smi. Put the LoadRoot before the JumpIfSmi(rdx...). It will be unused if both operands are smis, but most of the time we shouldn't be converting to double if both are smis anyway. This might be the cause of going to runtime more, since we misrecognize the HeapNumber. http://codereview.chromium.org/2862004/diff/4001/5001#newcode9963 src/x64/codegen-x64.cc:9963: __ j(equal, &load_float_rax); Why not put load_float_rax here, with a jmp to done after it, instead of a conditional jump followed by an unconditional one? http://codereview.chromium.org/2862004/diff/4001/5001#newcode9964 src/x64/codegen-x64.cc:9964: __ jmp(not_numbers); // Argument in eax is not a number. eax->rax http://codereview.chromium.org/2862004/diff/4001/5001#newcode10409 src/x64/codegen-x64.cc:10409: if (static_operands_type_.IsSmi()) { If we have HasSmiCodeInStub, this case should have been handled, and otherwise we wouldn't call the stub at all with two smis. Could we check if this case is ever hit? http://codereview.chromium.org/2862004/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
