LGTM
http://codereview.chromium.org/1860001/diff/1/5 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/1860001/diff/1/5#newcode7404 src/x64/codegen-x64.cc:7404: // Test that eax is a number. eax -> rax Move to after the label declarations? http://codereview.chromium.org/1860001/diff/1/5#newcode7413 src/x64/codegen-x64.cc:7413: ASSERT_EQ(1, kSmiTagSize); Does the assert add anything when you are using the SmiToInteger32 macro or should we just remove it? http://codereview.chromium.org/1860001/diff/1/5#newcode7427 src/x64/codegen-x64.cc:7427: __ cmpq(rbx, FieldOperand(rax, HeapObject::kMapOffset)); Can we use CompareRoot here? http://codereview.chromium.org/1860001/diff/1/5#newcode7443 src/x64/codegen-x64.cc:7443: // or h = (h0 ^ (h0 >> 8) ^ (h0 >> 16) ^ (h0 >> 24)) & cacheSize - 1 add parenthesis (cacheSize - 1)? http://codereview.chromium.org/1860001/diff/1/5#newcode7469 src/x64/codegen-x64.cc:7469: { // NOLINT - doesn't like a single brace on a line. Will the linter be happy with: { TranscendentalCache::Element ...; ... } ? http://codereview.chromium.org/1860001/diff/1/5#newcode7485 src/x64/codegen-x64.cc:7485: __ lea(rcx, Operand(rax, rcx, times_8, 0));\ Remove '\' at end of line. http://codereview.chromium.org/1860001/diff/1/5#newcode7497 src/x64/codegen-x64.cc:7497: // We are short on registers, so use no_reg as scratch. This is a left over comment from ia32, remove? http://codereview.chromium.org/1860001/diff/1/5#newcode7526 src/x64/codegen-x64.cc:7526: // Only free register is edi. rdi http://codereview.chromium.org/1860001/diff/1/5#newcode7540 src/x64/codegen-x64.cc:7540: __ andl(rdi, Immediate(0x7ff)); Should we use a couple of named constants for this? http://codereview.chromium.org/1860001/diff/1/5#newcode7542 src/x64/codegen-x64.cc:7542: (63 + HeapNumber::kExponentBias); This will fit on line above? http://codereview.chromium.org/1860001/diff/1/5#newcode7552 src/x64/codegen-x64.cc:7552: __ movq(rdi, static_cast<uint64_t>(0x7ff8)<<48, RelocInfo::NONE); We should use a named constant for the nan representation. http://codereview.chromium.org/1860001/diff/1/5#newcode7584 src/x64/codegen-x64.cc:7584: __ testl(rax, Immediate(0x400 /* C2 */)); We usually do not put comment in here. This seems like something where we should have a named constant. Or at least move the comment above the testl line and say 'extract C2 field'. http://codereview.chromium.org/1860001/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
