X64: Extract all smi operations into MacroAssembler macros.

X64: Extract all smi operations into MacroAssembler macros.
First step in changing Smi representation.

[Lasse Reichstein, 2009-09-10 09:38:59]

Review, and any suggestions for abstracting indexing by a smi, please.

[William Hesse, 2009-09-10 11:13:22]

LGTM.  Is there a performance impact?

http://codereview.chromium.org/196077/diff/1/5
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/196077/diff/1/5#newcode358
Line 358: __ PositiveSmiTimesPowerOfTwoToInteger64(rdx, rdx, kPointerSizeLog2);
Does this assume that the smi is zero-extended?  It looks to me that this is
guaranteed only by the fact that rax is zero-extended on entry to this entire
function.

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

http://codereview.chromium.org/196077/diff/1/6#newcode727
Line 727: Condition is_smi = masm_->CheckSmi(receiver.reg());
This bugged me a lot at first, but now I see that different checks might return
different conditions.

Why not put it on one line:
build_args.Branch(masm_->CheckSmi(receiver.reg()));

For the most common case, this one, I would define 
BranchIfSmi(reg);

http://codereview.chromium.org/196077/diff/1/6#newcode5390
Line 5390: __ SmiShiftLogicRightConstant(answer.reg(),
ShiftLogicalRight

[Lasse Reichstein, 2009-09-10 12:28:11]

Performance is approx. the same (~0.2% increase on V8 benchmark suite).

http://codereview.chromium.org/196077/diff/1/5
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/196077/diff/1/5#newcode358
Line 358: __ PositiveSmiTimesPowerOfTwoToInteger64(rdx, rdx, kPointerSizeLog2);
It should not assume anything except that rdx contains a positive smi. If any
code above makes assumptions on the representation of a smi, it should be
changed.

This should be __PositiveSmi...er64(rdx, rax, ...) and omit the mov before.

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

http://codereview.chromium.org/196077/diff/1/6#newcode727
Line 727: Condition is_smi = masm_->CheckSmi(receiver.reg());
I thought about inlining the CheckSmi call in the branch, but didn't like to
have an argument computation with side effects (it emits code).

I have avoided extending JumpTarget, but it would make sense to have a few
frequently used branch types directly on the JumpTarget (and perhaps on deferred
code too).

http://codereview.chromium.org/196077/diff/1/6#newcode5390
Line 5390: __ SmiShiftLogicRightConstant(answer.reg(),
Will rename LogicRight into LogicalRight.

[Lasse Reichstein, 2009-09-10 12:45:35]

http://codereview.chromium.org/196077/diff/1/5
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/196077/diff/1/5#newcode358
Line 358: __ PositiveSmiTimesPowerOfTwoToInteger64(rdx, rdx, kPointerSizeLog2);
Ah, you were referring to the implementation. That did miss a zero extension in
the general case.

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

http://codereview.chromium.org/196077/diff/1/6#newcode727
Line 727: Condition is_smi = masm_->CheckSmi(receiver.reg());
... but I'll leave that for a later change (e.g. when abstracting indexing using
a smi).