X64: Abstract indexing by a smi to the macro assembler.

X64: Abstract indexing by a smi to the macro assembler.

[Lasse Reichstein, 2009-09-15 09:26:47]

Smallish review.

[William Hesse, 2009-09-15 10:12:08]

http://codereview.chromium.org/196118/diff/1/3
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/196118/diff/1/3#newcode1102
Line 1102: return SmiIndex(src, static_cast<ScaleFactor>(shift - kSmiTagSize));
How are we making sure that src is zero-extended here?  Won't this cause errors?
Also, shouldn't we check 0 <= shift-kSmiTagSize < 4 ?

http://codereview.chromium.org/196118/diff/1/4
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/196118/diff/1/4#newcode44
Line 44: struct SmiIndex {
Are we allowed to create constructors for structs, according to the C++ style
guide?

http://codereview.chromium.org/196118/diff/1/4#newcode354
Line 354: // src is always unchanged.
Don't those two sentences contradict each other?  How could the returned
register be src, if src is not dst, and src must be unchanged?

http://codereview.chromium.org/196118/diff/1/5
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/196118/diff/1/5#newcode56
Line 56: __ cmpl(name, Operand(kScratchRegister, index.reg, index.scale, 0));
Document that offset must be zero-extended in the calling context of ProbeTable,
or fix it here.

[Lasse Reichstein, 2009-09-15 10:39:08]

http://codereview.chromium.org/196118/diff/1/3
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/196118/diff/1/3#newcode1102
Line 1102: return SmiIndex(src, static_cast<ScaleFactor>(shift - kSmiTagSize));
We should make sure. I'll add a movel(dst, src).

This code assumes the value of kSmiTagSize is 1. If that changes, all the
abstraced smi operations must be checked.

http://codereview.chromium.org/196118/diff/1/4
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/196118/diff/1/4#newcode44
Line 44: struct SmiIndex {
Yes (I had to check to be sure).

http://codereview.chromium.org/196118/diff/1/4#newcode354
Line 354: // src is always unchanged.
It can happen if an unchanged src combined with a ScaleFactor is sufficient.
Then dst is not written, and src i returned unmodified together with (typically)
times_4.
(However, after handling zero-extension, the returned register will, in
practice, always be dst).

http://codereview.chromium.org/196118/diff/1/5
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/196118/diff/1/5#newcode56
Line 56: __ cmpl(name, Operand(kScratchRegister, index.reg, index.scale, 0));
Fixed in SmiToIndex instead. 
I'll document that offset must be a positive smi.

[William Hesse, 2009-09-15 10:59:55]

LGTM.