http://codereview.chromium.org/6682026/diff/1/src/x64/assembler-x64.cc File src/x64/assembler-x64.cc (right):
http://codereview.chromium.org/6682026/diff/1/src/x64/assembler-x64.cc#newcode1603 src/x64/assembler-x64.cc:1603: movq(dst, reinterpret_cast<int64_t>(value), RelocInfo::NONE); Please consider using LoadSmi instead. It tries to optimize small smis by lea's on kSmiConstantRegister, and otherwise defaults to loading it verbatim like this. http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc#newcode2254 src/x64/code-stubs-x64.cc:2254: Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR)); You can use Cmp(const Operand* dst, Handle<Object> source) here. If you don't want to make a handle, consider renaming this to just Cmp(..Operand.., Smi*). The "SmiX" operations all work on operands that are both smis. http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc#newcode4562 src/x64/code-stubs-x64.cc:4562: __ nop(); Why the nop? http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc#newcode2249 src/x64/codegen-x64.cc:2249: __ cmpq(left_side.reg(), right_side.reg()); Why not SmiCompare? You are assuming the layout of smis outside of Smi code, please try to avoid that (if it really seems necessary, maybe it's reason to introduce another Smi-macro). Only use cmpq for pointer-equality test, not any of greater/less than. http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc#newcode6810 src/x64/codegen-x64.cc:6810: deferred->Branch(below_equal); Do use SmiCompare! http://codereview.chromium.org/6682026/diff/1/src/x64/full-codegen-x64.cc File src/x64/full-codegen-x64.cc (right): http://codereview.chromium.org/6682026/diff/1/src/x64/full-codegen-x64.cc#newcode3035 src/x64/full-codegen-x64.cc:3035: __ cmpq(temp, index_2); Do use SmiCompare. http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc File src/x64/macro-assembler-x64.cc (left): http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#oldcode841 src/x64/macro-assembler-x64.cc:841: cmpq(dst, src); Don't remove the SmiCompare function. Do use it when you compare smis for anything but equality! In general, don't make assumptions (in genrated code) about the representation of smis outside of the MacroAssembler smi code. http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#newcode865 src/x64/macro-assembler-x64.cc:865: } Adding asserts: Good! http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#newcode893 src/x64/macro-assembler-x64.cc:893: cmpq(dst, kScratchRegister); This is wasteful, since it doesn't use the kSmiConstantRegister for the values where it can be used. It's better to use: Register const_reg = GetSmiConstant(kScratchRegister, src); cmpq(dst, const_reg); as in SmiCompare(Register,Smi*). http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h File src/x64/macro-assembler-x64.h (right): http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h#newcode281 src/x64/macro-assembler-x64.h:281: // Simple comparison of Smis. Actually, I believe we try to write "smi" as lower-case when referring to the concept, not the Class. http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h#newcode288 src/x64/macro-assembler-x64.h:288: void SmiCompareWithObject(const Operand& dst, Smi* src); Rename them to Cmp and group them with the Cmp functions instead. http://codereview.chromium.org/6682026/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
