I have addressed all the comments. Now I will split this CL into two as Mads
suggested: one dealing with registers/smis improvements, another dealing
with
ICs.
http://codereview.chromium.org/551093/diff/1/3
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/551093/diff/1/3#newcode744
src/ia32/codegen-ia32.cc:744: enum ArgsLocation {
On 2010/01/22 09:01:18, Erik Corry wrote:
I would prefer ArgLocation.
Done.
http://codereview.chromium.org/551093/diff/1/3#newcode760
src/ia32/codegen-ia32.cc:760: // Returns operands as floating point
numbers on FPU stack.
On 2010/01/22 09:01:18, Erik Corry wrote:
No capital letter after a comma.
Done.
http://codereview.chromium.org/551093/diff/1/3#newcode789
src/ia32/codegen-ia32.cc:789: // Accepts operands on stack or in
eax,ebx.
On 2010/01/22 09:01:18, Erik Corry wrote:
Space after comma.
Done.
http://codereview.chromium.org/551093/diff/1/3#newcode799
src/ia32/codegen-ia32.cc:799: case BINARY_OP_STUB_GENERIC: return
"Universal"; break;
On 2010/01/22 09:01:18, Erik Corry wrote:
You don't need a break after a return.
Done.
http://codereview.chromium.org/551093/diff/1/3#newcode1398
src/ia32/codegen-ia32.cc:1398: __ add(answer.reg(),
Operand(right->reg()));
There is a smi check several lines above that code, and the deferred
code does not undo the operation, so the operation is not really
optimistic.
On 2010/01/22 09:01:18, Erik Corry wrote:
I think the comment should go back.
http://codereview.chromium.org/551093/diff/1/3#newcode7265
src/ia32/codegen-ia32.cc:7265: ASSERT(kSmiTag == 0); // adjust zero
check if not the case
On 2010/01/22 09:01:18, Erik Corry wrote:
adjust -> Adjust
case -> case.
Done.
http://codereview.chromium.org/551093/diff/3002/3012
File src/debug.cc (right):
http://codereview.chromium.org/551093/diff/3002/3012#newcode129
src/debug.cc:129: && code->kind() != Code::BINARY_OP_IC) ||
On 2010/01/22 12:14:26, Mads Ager wrote:
I would put the '&&' on the previous line.
if ((code->is_inline_cache_stub() &&
code->kind() != Code::BINARY_OP_IC) ||
RelocInfo::Is...(rmode))) {
Done.
http://codereview.chromium.org/551093/diff/3002/3004
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/551093/diff/3002/3004#newcode766
src/ia32/codegen-ia32.cc:766: // Accepts operands on stack or in
eax,ebx.
On 2010/01/22 12:14:26, Mads Ager wrote:
Space after comma.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode789
src/ia32/codegen-ia32.cc:789: // Accepts operands on stack or in
eax,ebx.
On 2010/01/22 12:14:26, Mads Ager wrote:
Space after comma.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode799
src/ia32/codegen-ia32.cc:799: case BINARY_OP_STUB_GENERIC: return
"Universal"; break;
On 2010/01/22 12:14:26, Mads Ager wrote:
"Universal" -> "Generic"
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7260
src/ia32/codegen-ia32.cc:7260: case Token::ADD: __ sub(eax,
Operand(ebx)); break;
On 2010/01/22 12:14:26, Mads Ager wrote:
Add comment that says that this reverts the optimistic operation.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7277
src/ia32/codegen-ia32.cc:7277: __ AllocateHeapNumber(edx, ecx, no_reg,
On 2010/01/22 12:14:26, Mads Ager wrote:
Reindent argument list:
__ AllocateHeapNumber(
edx,
ecx,
no_reg,
HasArgumentsInRegister() ? ...);
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7314
src/ia32/codegen-ia32.cc:7314: // These operations always succeed on a
pair of smis.
We never get to this place when executing, but we can get here while
generating. Inserted "Do nothing here" at the start of the comment to
make it (hopefully) clearer.
Fixed indentation.
On 2010/01/22 12:14:26, Mads Ager wrote:
That means that we should never get to this place, right? In that
case, please
put in a call to UNREACHABLE() here.
Also, indentation seems off by one here. Indented by three instead of
two?
http://codereview.chromium.org/551093/diff/3002/3004#newcode7319
src/ia32/codegen-ia32.cc:7319: // These go directly to runtime
See previous reply.
On 2010/01/22 12:14:26, Mads Ager wrote:
Similarly here: UNREACHABLE()?
Three-space indented instead of two?
http://codereview.chromium.org/551093/diff/3002/3004#newcode7339
src/ia32/codegen-ia32.cc:7339: default: UNREACHABLE(); break;
On 2010/01/22 12:14:26, Mads Ager wrote:
Indentation is off for the cases in this switch. The end '}' should
align with
the 'default:'. It seems the case is four-space indented instead of
two-space
indented?
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7419
src/ia32/codegen-ia32.cc:7419: // For MOD we go directly to runtime in
the non-smi case.
Similarly, this line is executed on code generation, but no code needs
to be generated. Removing this case will case the default branch call
UNREACHABLE().
On 2010/01/22 12:14:26, Mads Ager wrote:
UNREACHABLE()?
http://codereview.chromium.org/551093/diff/3002/3004#newcode7506
src/ia32/codegen-ia32.cc:7506: // are already in edx,eax
On 2010/01/22 12:14:26, Mads Ager wrote:
Space after comma.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7516
src/ia32/codegen-ia32.cc:7516: // First argument is a a string, test
second.
On 2010/01/22 12:14:26, Mads Ager wrote:
a a -> a
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7570
src/ia32/codegen-ia32.cc:7570: break;
On 2010/01/22 12:14:26, Mads Ager wrote:
indentation.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7599
src/ia32/codegen-ia32.cc:7599: // Generate the unreachable reference to
the original stub so that it can be
On 2010/01/22 12:14:26, Mads Ager wrote:
the -> an
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7625
src/ia32/codegen-ia32.cc:7625: (fast_case_ ==
BINARY_OP_STUB_UNINITIALIZED)) {
On 2010/01/22 12:14:26, Mads Ager wrote:
Indent one more space.
Could you add a comment explaining the ShouldGenerateSmiCode() part of
this
condition?
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7654
src/ia32/codegen-ia32.cc:7654: __ push(eax);
On 2010/01/22 12:14:26, Mads Ager wrote:
Add comment explaining what eax is here to make this readable as a
standalone
method.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7656
src/ia32/codegen-ia32.cc:7656: // __ push(eax);
On 2010/01/22 12:14:26, Mads Ager wrote:
Code in comment.
Done.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7680
src/ia32/codegen-ia32.cc:7680: __ pop(eax);
There is always a tail call following this so we never break anything.
Added a comment clarifying this.
On 2010/01/22 12:14:26, Mads Ager wrote:
Popping the arguments instead of loading them seems dangerous to me.
After
popping, there will be two missing arguments on the stack under the
return
address - does the return code handle that correctly or will we pop
again on
return?
Or do we always tail-call something at some later point so that this
is safe?
We should see if we can make this clearer and at least add comments
explaining
why this is ok.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7762
src/ia32/codegen-ia32.cc:7762: Label* alloc_failure) {
On 2010/01/22 12:14:26, Mads Ager wrote:
Indentation.
Done.
http://codereview.chromium.org/551093/diff/3002/3005
File src/ia32/codegen-ia32.h (right):
http://codereview.chromium.org/551093/diff/3002/3005#newcode704
src/ia32/codegen-ia32.h:704: // Minor key encoding in 16 bits
CCCFRASOOOO0OOOMM.
On 2010/01/22 12:14:26, Mads Ager wrote:
I think there is a zero '0' instead of the letter 'O' here?
Made sure those are letter 'O's
http://codereview.chromium.org/551093/diff/3002/3005#newcode751
src/ia32/codegen-ia32.h:751: return HasSmiCodeInStub() && (fast_case_ <
BINARY_OP_STUB_FAST_FP);
This is taken care of in GetFastCase method. It makes sure that if
NO_SMI_CODE_IN_STUB is set the FAST_FP stub is never generated (GENERIC
is generated insted).
On 2010/01/22 12:14:26, Mads Ager wrote:
This is not really related to this method itself, but can we get in a
situation
where HasSmiCodeInStub() returns true and fast_case_ ==
BINARY_OP_STUB_FAST_FP?
We should make sure that we cannot get in that situation to make sure
that we do
not get two different stubs with the same code.
I guess we only want the Smi code for the generic stub? Test
explicitly for
fast_case_ == BINARY_OP_STUB_GENERIC?
http://codereview.chromium.org/551093/diff/3002/3008
File src/ic.cc (right):
http://codereview.chromium.org/551093/diff/3002/3008#newcode1460
src/ic.cc:1460: Object * BinaryOp_Patch(Arguments args) {
On 2010/01/22 12:14:26, Mads Ager wrote:
Remove space before '*'.
Done.
http://codereview.chromium.org/551093
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
