X64: Convert smis to holding 32 bits of payload.

X64: Convert smis to holding 32 bits of payload.

[William Hesse, 2009-09-21 13:31:36]

Mainly formatting issues.

http://codereview.chromium.org/196139/diff/1/7
File src/objects.h (right):

http://codereview.chromium.org/196139/diff/1/7#newcode908
Line 908: static inline bool IsValid(intptr_t value);
Are there unit tests verifying the correct behavior on unsigned int?  I assume
an unsigned 32-bit int between 2^31 and 2^32 should not be a valid Smi.

http://codereview.chromium.org/196139/diff/1/8
File src/utils.h (right):

http://codereview.chromium.org/196139/diff/1/8#newcode39
Line 39: // Returns true iff x is a power of 2.  Does not work for zero or max
negative.
Why doesn't this work for the minimum negative number?  It looks to me like it
does.

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

http://codereview.chromium.org/196139/diff/1/14#newcode771
Line 771: __ SmiCompare(rcx, Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR));
If we had a SmiCompare(Operand, Smi), then we could optimize it in the 32-bit
smi case to only read the top 32 bits, and compare directly with an immediate.

http://codereview.chromium.org/196139/diff/1/16
File src/x64/macro-assembler-x64.cc (left):

http://codereview.chromium.org/196139/diff/1/16#oldcode48
Line 48: 
We use two lines between function definitions in a .cc file.

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

http://codereview.chromium.org/196139/diff/1/16#newcode65
Line 65: Immediate(static_cast<int32_t> (~Page::kPageAlignmentMask)));
What is this space here for?

http://codereview.chromium.org/196139/diff/1/16#newcode243
Line 243: intptr_t p1 = reinterpret_cast<intptr_t> (msg);
C++ style guide (for open-source, at googlecode.com) says no space between > and
( in a cast.

http://codereview.chromium.org/196139/diff/1/16#newcode264
Line 264: ASSERT(allow_stub_calls()); // calls are not allowed in some stubs
Need two spaces between ; and //.

http://codereview.chromium.org/196139/diff/1/16#newcode1039
Line 1039: #else
Comment this else, or else put a big header comment" ___ END OF 32-BIT SMI CODE,
BEGINNING OF 31-BIT SMI CODE _______**********

http://codereview.chromium.org/196139/diff/1/16#newcode1249
Line 1249: Register src1,
Why aren't the arguments aligned with each other?  I'm pretty sure that is the
standard we use throughout V8.

http://codereview.chromium.org/196139/diff/1/16#newcode1891
Line 1891: SizeOfCodeGeneratedSince(&target) + kPointerSize);
C++ style guide says that function definitions, declarations, and calls are
wrapped at the parenthesis, unless it is impossible to fit within the line
length.

http://codereview.chromium.org/196139/diff/1/16#newcode1948
Line 1948: push(Immediate(0)); // NULL frame pointer.
This will not lint.  Need two spaces before //.

http://codereview.chromium.org/196139/diff/1/16#newcode1956
Line 1956: 
Put two blank lines between function definitions in a .cc file.

http://codereview.chromium.org/196139/diff/1/16#newcode2741
Line 2741: }
Don't change this to be different from all the other V8 files.

http://codereview.chromium.org/196139/diff/1/21
File test/mjsunit/smi-negative-zero.js (right):

http://codereview.chromium.org/196139/diff/1/21#newcode50
Line 50: assertEquals(-Infinity, one / (minus_four % two), "foo1");
If we are fixing this up, shouldn't we replace "foo1" with
"one / (minus_four % two)"?

[Lasse Reichstein, 2009-09-30 08:56:17]

Now works. Benchmarks are still running. 
Last chance to compare to previous upload before I merge with head.

http://codereview.chromium.org/196139/diff/1/7
File src/objects.h (right):

http://codereview.chromium.org/196139/diff/1/7#newcode908
Line 908: static inline bool IsValid(intptr_t value);
Correct, smi values are always a two's complement interpretation of a number of
bits (either 31 or 32). 
There are currently no tests. That should be changed.

http://codereview.chromium.org/196139/diff/1/8
File src/utils.h (right):

http://codereview.chromium.org/196139/diff/1/8#newcode39
Line 39: // Returns true iff x is a power of 2.  Does not work for zero or max
negative.
Minimum negative number - 1 overflows, statically, so we can't compile it with
warnings as errors.

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

http://codereview.chromium.org/196139/diff/1/14#newcode771
Line 771: __ SmiCompare(rcx, Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR));
We actually do have a SmiCompare(Operand&, Smi*), but we have no (reasonable)
way to add two to the address of an Operand after it has been created.
We should still use the direct function as it doesn't seem that rcx is used
again.

http://codereview.chromium.org/196139/diff/1/16
File src/x64/macro-assembler-x64.cc (left):

http://codereview.chromium.org/196139/diff/1/16#oldcode48
Line 48: 
Done.

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

http://codereview.chromium.org/196139/diff/1/16#newcode65
Line 65: Immediate(static_cast<int32_t> (~Page::kPageAlignmentMask)));
Something has tried to reformat my files. Perhaps VisualStudio (who knows what
buttons I might have pressed).
It's a bug and will be fixed.

http://codereview.chromium.org/196139/diff/1/16#newcode243
Line 243: intptr_t p1 = reinterpret_cast<intptr_t> (msg);
Another automatic formatting bug. Sigh. Never turn your back on an IDE.

http://codereview.chromium.org/196139/diff/1/16#newcode264
Line 264: ASSERT(allow_stub_calls()); // calls are not allowed in some stubs
Also fixed everywhere.

http://codereview.chromium.org/196139/diff/1/16#newcode1039
Line 1039: #else
I'll just remove the entire else branch. It's double work keeping it up-to-date,
and we won't need both versions anyway.

http://codereview.chromium.org/196139/diff/1/16#newcode1249
Line 1249: Register src1,
Would you believe the same excuse for the fourth time in a row. That too will be
fixed.

http://codereview.chromium.org/196139/diff/1/16#newcode1891
Line 1891: SizeOfCodeGeneratedSince(&target) + kPointerSize);
Fixing.

http://codereview.chromium.org/196139/diff/1/16#newcode1948
Line 1948: push(Immediate(0)); // NULL frame pointer.
Done.

http://codereview.chromium.org/196139/diff/1/16#newcode1956
Line 1956: 
Done.

http://codereview.chromium.org/196139/diff/1/16#newcode2741
Line 2741: }
Done.

http://codereview.chromium.org/196139/diff/1/21
File test/mjsunit/smi-negative-zero.js (right):

http://codereview.chromium.org/196139/diff/1/21#newcode50
Line 50: assertEquals(-Infinity, one / (minus_four % two), "foo1");
Personally, I like "foo1" better - it's easier to find by searching than some
complex but meaningfull string.
All I want is an id to match a failure against the code that generated it. If
the id has meaning as well, it can get out of sync.

[Lasse Reichstein, 2009-10-02 09:58:22]

Ready for re-review.
The 31-bit smis are currently slightly broken, but the 32-bit ones work
correctly, and, on average, slightly faster.

[William Hesse, 2009-10-02 12:57:41]

Reviewed up through builtins-x64.cc.

http://codereview.chromium.org/196139/diff/10001/11009
File src/runtime.cc (right):

http://codereview.chromium.org/196139/diff/10001/11009#newcode3243
Line 3243: // We don't allow strings that are longer than Smi range.
Comment refers to Smi range.

http://codereview.chromium.org/196139/diff/10001/11012
File src/utils.h (right):

http://codereview.chromium.org/196139/diff/10001/11012#newcode39
Line 39: // Returns true iff x is a power of 2.  Does not work for zero or max
negative.
Returns true iff x is a power of 2, zero, or or the minimum integer.

[Lasse Reichstein, 2009-10-05 05:24:29]

Erik, could you take a look at the 64-bit Smi operations in
macroassembler-x64.cc too, with an eye on possible optimizations?

[Erik Corry, 2009-10-06 19:44:57]

LGTM, though my brain did boil over when I got to the 31 bit versions of
everything.

http://codereview.chromium.org/196139/diff/13001/9021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9021#newcode138
Line 138: PrintF(
Formatting looks wrong here.

http://codereview.chromium.org/196139/diff/13001/9021#newcode144
Line 144: // Minor key encoding in 12 bits of three registers (object, address
and
Hmmm.  Are we really interested in generating 4096 different versions of this
stub.  I hope we generate much fewer in practice...

http://codereview.chromium.org/196139/diff/13001/9021#newcode202
Line 202: movq(kScratchRegister, ExternalReference::new_space_start());
No need for a scratch register here.  You can just compare with the external
reference.

http://codereview.chromium.org/196139/diff/13001/9021#newcode397
Line 397: int builtins_offset = JSBuiltinsObject::kJSBuiltinsOffset + (id
This formatting change makes the code less readable.

http://codereview.chromium.org/196139/diff/13001/9021#newcode570
Line 570: testl(kScratchRegister, Immediate(kSmiTagMask));
testb

http://codereview.chromium.org/196139/diff/13001/9021#newcode676
Line 676: or_(dst, src2);
Couldn't this be an xor?

http://codereview.chromium.org/196139/diff/13001/9021#newcode699
Line 699: or_(kScratchRegister, src2);
And here?

http://codereview.chromium.org/196139/diff/13001/9021#newcode721
Line 721: testl(tmp, Immediate(kSmiTagMask));
testb?

http://codereview.chromium.org/196139/diff/13001/9021#newcode726
Line 726: cmovq(overflow, dst, kScratchRegister);
According to Torvalds you should only use cmov if the alternative is a branch
that can't be predicted.  But this branch is easy to predict.

http://codereview.chromium.org/196139/diff/13001/9021#newcode773
Line 773: if (constant->value() == 0) {
Somewhat inconsistent that we don't do this optimization on the other cases
above.  Are we addressing a concrete situation here? Should we add it above too?

http://codereview.chromium.org/196139/diff/13001/9021#newcode784
Line 784: // Notice: This works even for Smi::kMinValue.
Comment seems to contradict code.

http://codereview.chromium.org/196139/diff/13001/9021#newcode1104
Line 1104: orl(rcx, Immediate(kSmiShift));
Funky!

[Søren Thygesen Gjesse, 2009-10-06 21:25:36]

Drive by style related comments.

http://codereview.chromium.org/196139/diff/13001/9021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9021#newcode123
Line 123: class RecordWriteStub: public CodeStub {
According to the style guide
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...
the space should actually be there (I don't think we are very consistent here
though).

http://codereview.chromium.org/196139/diff/13001/9021#newcode146
Line 146: class ScratchBits: public BitField<uint32_t, 0, 4> {
Why do you want newlines here?

http://codereview.chromium.org/196139/diff/13001/9021#newcode435
Line 435: #if V8_LONG_SMI
I think we normally use #ifdef without caring about the actal value for this
type conditional compilation.

http://codereview.chromium.org/196139/diff/13001/9021#newcode1221
Line 1221: #else  // 31 BIT SMI OPERATIONS.
#else  // V8_LONG_SMI
Move comment to new line and DONT USE CAPS :-).

http://codereview.chromium.org/196139/diff/13001/9021#newcode1952
Line 1952: #endif
#endif  // V8_LONG_SMI
(if this is the correct one that matches #ifdef V8_LONG_SMI)

[William Hesse, 2009-10-07 11:45:02]

Some more comments, I am still reviewing.

http://codereview.chromium.org/196139/diff/13001/9019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9019#newcode799
Line 799: __ bind(&loop);
Actually, wouldn't the loop prefix work here? loop push ...
I don't think we've implemented it in the assembler, though.
(the loop prefix runs an intstruction, decrements ecx, and loops if nonzero).

http://codereview.chromium.org/196139/diff/13001/9019#newcode4952
Line 4952: __ SmiCompare(left_side.reg(), Smi::cast(*right_side.handle()));
Couldn't the changes to SmiCompare and removing Immediate(Smi) have been done in
a separate change list, before introducing the 32-bit smis?

http://codereview.chromium.org/196139/diff/13001/9019#newcode5005
Line 5005: __ SmiTest(answer.reg());  // Both zero and sign flag right.
Make this comment the same as the one below.

http://codereview.chromium.org/196139/diff/13001/9019#newcode5313
Line 5313: Smi::FromInt(int_value),
Why not use smi_value here?

http://codereview.chromium.org/196139/diff/13001/9019#newcode5335
Line 5335: Smi::FromInt(int_value),
Same.

http://codereview.chromium.org/196139/diff/13001/9019#newcode5462
Line 5462: Smi::FromInt(int_value));
Same.

http://codereview.chromium.org/196139/diff/13001/9019#newcode6489
Line 6489: __ cmovq(below, rax, rdx);  // -1 if negative.
I think we have gone back and forth on this.  This should be a separate change,
so its effect on performance can be measured.

http://codereview.chromium.org/196139/diff/13001/9019#newcode7565
Line 7565: __ JumpIfNotValidSmiValue(rax, &non_smi_result);
I don't see why this test is needed.  Are we supporting both 32 and 31-bit smis?
 I think the test goes away for 32-bit smis.

[William Hesse, 2009-10-07 14:38:34]

Still not done - here are more comments.  It might be good to separate out the
formatting changes from the real changes, to make two changelists.

http://codereview.chromium.org/196139/diff/13001/9021
File src/x64/macro-assembler-x64.cc (left):

http://codereview.chromium.org/196139/diff/13001/9021#oldcode2185
Line 2185: 
What do all the other files in v8 do?

http://codereview.chromium.org/196139/diff/13001/9021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9021#newcode30
Line 30: #include "bootstrapper.h"
Change title of this changelist.  It still says "not ready for review"

http://codereview.chromium.org/196139/diff/13001/9021#newcode44
Line 44: allow_stub_calls_(true), code_object_(Heap::undefined_value()) {
Why are these two initializers on the same line?

http://codereview.chromium.org/196139/diff/13001/9021#newcode160
Line 160: | AddressBits::encode(addr_.code())
We have been putting operators at the end of the previous line.

http://codereview.chromium.org/196139/diff/13001/9021#newcode249
Line 249: Check(cc, msg);
If the body is on a separate line, it needs braces around it.

http://codereview.chromium.org/196139/diff/13001/9021#newcode262
Line 262: void MacroAssembler::NegativeZeroTest(Register result, Register op,
If one line break is needed, then align all parameters.

http://codereview.chromium.org/196139/diff/13001/9021#newcode398
Line 398: * kPointerSize);
Break lines at the outermost operator that you can.

http://codereview.chromium.org/196139/diff/13001/9021#newcode848
Line 848: SmiToInteger32(rax, src1);
Could you leave rax with the smi shift, divide by int src2, and check both
remainder rdx and the low bits eax for zero,
and have the result already smi-encoded?

http://codereview.chromium.org/196139/diff/13001/9021#newcode1222
Line 1222: 
Could these even be moved to two separate files, smi_ops_long_smis.cc and
smi_ops_31bit_smis.cc?  That would make the parallel structure more readable. 
Then you could just conditionally include one.

http://codereview.chromium.org/196139/diff/13001/9021#newcode1367
Line 1367: cmpl(src, Immediate(0x80000000u));
Was this an actual error that was fixed?

http://codereview.chromium.org/196139/diff/13001/9021#newcode2257
Line 2257: << Map::kHasNonInstancePrototype));
Break lines at the outermost possible operator.

http://codereview.chromium.org/196139/diff/13001/9021#newcode2390
Line 2390: RegList regs) {
Did these exceed 80 characters when they were aligned?

http://codereview.chromium.org/196139/diff/13001/9021#newcode2454
Line 2454: == SharedFunctionInfo::kDontAdaptArgumentsSentinel) {
We have broken lines after the operator in all the other v8 code.

http://codereview.chromium.org/196139/diff/13001/9021#newcode2544
Line 2544: SharedFunctionInfo::kFormalParameterCountOffset));
Not outermost.

http://codereview.chromium.org/196139/diff/13001/9021#newcode2846
Line 2846: Context::kHeaderSize + Context::SECURITY_TOKEN_INDEX * kPointerSize;
I like this change.

http://codereview.chromium.org/196139/diff/13001/9021#newcode3014
Line 3014: : address_(address),
I think the style guide says 4 spaces before the :.  Check it.

[William Hesse, 2009-10-08 10:11:34]

LGTM!

http://codereview.chromium.org/196139/diff/13001/9028
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/196139/diff/13001/9028#newcode148
Line 148: 
Two consecutive blank lines inside a function is not encouraged.

http://codereview.chromium.org/196139/diff/13001/9029
File test/cctest/test-macro-assembler-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9029#newcode87
Line 87: 
Can you use register aliases Param1, Param2, etc., so that the test will work on
Windows as well?

http://codereview.chromium.org/196139/diff/13001/9029#newcode1077
Line 1077: bool overflow = (x == Smi::kMinValue) && (y < 0);  // Safe approx.
used.
Inconsistent use of parentheses compared to line above.  I would parenthesize
the && relative to =.
Use of && to combine comparisons is so standard that I don't think the
parentheses need to be explicit.

http://codereview.chromium.org/196139/diff/13001/9029#newcode1081
Line 1081: bool fraction = !division_by_zero && !overflow && ((x % y) != 0);
Drop one set ().

http://codereview.chromium.org/196139/diff/13001/9030
File test/mjsunit/for-in.js (right):

http://codereview.chromium.org/196139/diff/13001/9030#newcode33
Line 33: 
This should be a separate change.

http://codereview.chromium.org/196139/diff/13001/9031
File test/mjsunit/regress/regress-1199401.js (right):

http://codereview.chromium.org/196139/diff/13001/9031#newcode33
Line 33: 
Good.

[Lasse Reichstein, 2009-10-08 11:02:06]

http://codereview.chromium.org/196139/diff/13001/9019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9019#newcode799
Line 799: __ bind(&loop);
It would, but it's a multi-micro-op instruction with a fairly large latency on
modern chips.
(The AMD64 optimization guide simply writes that you shouldn 't use loop.)

http://codereview.chromium.org/196139/diff/13001/9019#newcode4952
Line 4952: __ SmiCompare(left_side.reg(), Smi::cast(*right_side.handle()));
They could, and possibly should, but the SmiCompare functions were the final
result of a number of attempts of doing the 32-bit smis. They grew out of the
other changes.

http://codereview.chromium.org/196139/diff/13001/9019#newcode5005
Line 5005: __ SmiTest(answer.reg());  // Both zero and sign flag right.
Done.

http://codereview.chromium.org/196139/diff/13001/9019#newcode5313
Line 5313: Smi::FromInt(int_value),
Good point. Fixed.

http://codereview.chromium.org/196139/diff/13001/9019#newcode5335
Line 5335: Smi::FromInt(int_value),
Fixed.

http://codereview.chromium.org/196139/diff/13001/9019#newcode6489
Line 6489: __ cmovq(below, rax, rdx);  // -1 if negative.
Agree. I'll revert it for now, and look at it again later.

http://codereview.chromium.org/196139/diff/13001/9019#newcode7565
Line 7565: __ JumpIfNotValidSmiValue(rax, &non_smi_result);
We are supporting both.
The test does go away, for 32-bit smis, this is handled by the opcodes, so the
call doesn't actually add any code.

http://codereview.chromium.org/196139/diff/13001/9021
File src/x64/macro-assembler-x64.cc (left):

http://codereview.chromium.org/196139/diff/13001/9021#oldcode2185
Line 2185: 
Generally one line of whitespace before the namespace end.

http://codereview.chromium.org/196139/diff/13001/9021
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9021#newcode30
Line 30: #include "bootstrapper.h"
Will do.

http://codereview.chromium.org/196139/diff/13001/9021#newcode44
Line 44: allow_stub_calls_(true), code_object_(Heap::undefined_value()) {
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode123
Line 123: class RecordWriteStub: public CodeStub {
Ack. Something (likely a combination of me and one of my IDEs) did a
reformatting of this file at some time. 
I can see I haven't weeded out all its mistakes yet.

http://codereview.chromium.org/196139/diff/13001/9021#newcode138
Line 138: PrintF(
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode144
Line 144: // Minor key encoding in 12 bits of three registers (object, address
and
I think we do, but perhaps we should measure it, just to be sure. It does seem
like something that can be called with dynamically allocaated registers.

http://codereview.chromium.org/196139/diff/13001/9021#newcode146
Line 146: class ScratchBits: public BitField<uint32_t, 0, 4> {
Actually, I don't. Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode160
Line 160: | AddressBits::encode(addr_.code())
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode202
Line 202: movq(kScratchRegister, ExternalReference::new_space_start());
Sadly only by using the Cmp macro ... which does exaxtly the same as this
sequence.

http://codereview.chromium.org/196139/diff/13001/9021#newcode249
Line 249: Check(cc, msg);
Ack. Reformatting issues. I'll try to get them all fixed back this time.

http://codereview.chromium.org/196139/diff/13001/9021#newcode262
Line 262: void MacroAssembler::NegativeZeroTest(Register result, Register op,
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode397
Line 397: int builtins_offset = JSBuiltinsObject::kJSBuiltinsOffset + (id
Reverted.

http://codereview.chromium.org/196139/diff/13001/9021#newcode398
Line 398: * kPointerSize);
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode435
Line 435: #if V8_LONG_SMI
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode676
Line 676: or_(dst, src2);
Indeed it could. Well spotted.

http://codereview.chromium.org/196139/diff/13001/9021#newcode699
Line 699: or_(kScratchRegister, src2);
That too.

http://codereview.chromium.org/196139/diff/13001/9021#newcode721
Line 721: testl(tmp, Immediate(kSmiTagMask));
Method rewritten.

http://codereview.chromium.org/196139/diff/13001/9021#newcode726
Line 726: cmovq(overflow, dst, kScratchRegister);
I'll defer to his experience then.
The function can be greatly simplified in that case too.
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode773
Line 773: if (constant->value() == 0) {
We should. Done.

http://codereview.chromium.org/196139/diff/13001/9021#newcode784
Line 784: // Notice: This works even for Smi::kMinValue.
Comment removed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode848
Line 848: SmiToInteger32(rax, src1);
I did this originally, but using idivq was *significantly* slower than shifting
everything down, using idivl, and shifing it back up.

http://codereview.chromium.org/196139/diff/13001/9021#newcode1221
Line 1221: #else  // 31 BIT SMI OPERATIONS.
DONE!

http://codereview.chromium.org/196139/diff/13001/9021#newcode1222
Line 1222: 
It probably could, but in the long (and not even that long) run, I expect the
31-bit code to be going away entirely. We don't want to maintain both versions.

http://codereview.chromium.org/196139/diff/13001/9021#newcode1367
Line 1367: cmpl(src, Immediate(0x80000000u));
Yes. It was caught by the added test-macro-assembler-x64 file.

http://codereview.chromium.org/196139/diff/13001/9021#newcode1952
Line 1952: #endif
Done

http://codereview.chromium.org/196139/diff/13001/9021#newcode2257
Line 2257: << Map::kHasNonInstancePrototype));
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode2454
Line 2454: == SharedFunctionInfo::kDontAdaptArgumentsSentinel) {
Yes, I'll fix them all back.

http://codereview.chromium.org/196139/diff/13001/9021#newcode2544
Line 2544: SharedFunctionInfo::kFormalParameterCountOffset));
Fixed.

http://codereview.chromium.org/196139/diff/13001/9021#newcode3014
Line 3014: : address_(address),
It does. Fixed.

http://codereview.chromium.org/196139/diff/13001/9028
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/196139/diff/13001/9028#newcode148
Line 148: 
Fixed.

http://codereview.chromium.org/196139/diff/13001/9029
File test/cctest/test-macro-assembler-x64.cc (right):

http://codereview.chromium.org/196139/diff/13001/9029#newcode87
Line 87: 
I actually only ever use F0, so it should work.
I'll remove the other typedefs.

http://codereview.chromium.org/196139/diff/13001/9029#newcode1077
Line 1077: bool overflow = (x == Smi::kMinValue) && (y < 0);  // Safe approx.
used.
Done.

http://codereview.chromium.org/196139/diff/13001/9029#newcode1081
Line 1081: bool fraction = !division_by_zero && !overflow && ((x % y) != 0);
On 2009/10/08 10:11:34, William Hesse wrote:
> Drop one set ().

Done.

http://codereview.chromium.org/196139/diff/13001/9030
File test/mjsunit/for-in.js (right):

http://codereview.chromium.org/196139/diff/13001/9030#newcode33
Line 33: 
I don't think it's worth a separate change. 
It was just added to aid in debugging the current changes, and its both safe and
helpful to leave in.