First batch of comments.
We should probably measure the code size increase for the extra stubs types
on
real apps.
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) ||
I would put the '&&' on the previous line.
if ((code->is_inline_cache_stub() &&
code->kind() != Code::BINARY_OP_IC) ||
RelocInfo::Is...(rmode))) {
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.
Space after comma.
http://codereview.chromium.org/551093/diff/3002/3004#newcode789
src/ia32/codegen-ia32.cc:789: // Accepts operands on stack or in
eax,ebx.
Space after comma.
http://codereview.chromium.org/551093/diff/3002/3004#newcode799
src/ia32/codegen-ia32.cc:799: case BINARY_OP_STUB_GENERIC: return
"Universal"; break;
"Universal" -> "Generic"
http://codereview.chromium.org/551093/diff/3002/3004#newcode7260
src/ia32/codegen-ia32.cc:7260: case Token::ADD: __ sub(eax,
Operand(ebx)); break;
Add comment that says that this reverts the optimistic operation.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7277
src/ia32/codegen-ia32.cc:7277: __ AllocateHeapNumber(edx, ecx, no_reg,
Reindent argument list:
__ AllocateHeapNumber(
edx,
ecx,
no_reg,
HasArgumentsInRegister() ? ...);
http://codereview.chromium.org/551093/diff/3002/3004#newcode7314
src/ia32/codegen-ia32.cc:7314: // These operations always succeed on a
pair of smis.
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
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;
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?
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.
UNREACHABLE()?
http://codereview.chromium.org/551093/diff/3002/3004#newcode7506
src/ia32/codegen-ia32.cc:7506: // are already in edx,eax
Space after comma.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7516
src/ia32/codegen-ia32.cc:7516: // First argument is a a string, test
second.
a a -> a
http://codereview.chromium.org/551093/diff/3002/3004#newcode7570
src/ia32/codegen-ia32.cc:7570: break;
indentation.
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
the -> an
http://codereview.chromium.org/551093/diff/3002/3004#newcode7625
src/ia32/codegen-ia32.cc:7625: (fast_case_ ==
BINARY_OP_STUB_UNINITIALIZED)) {
Indent one more space.
Could you add a comment explaining the ShouldGenerateSmiCode() part of
this condition?
http://codereview.chromium.org/551093/diff/3002/3004#newcode7654
src/ia32/codegen-ia32.cc:7654: __ push(eax);
Add comment explaining what eax is here to make this readable as a
standalone method.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7656
src/ia32/codegen-ia32.cc:7656: // __ push(eax);
Code in comment.
http://codereview.chromium.org/551093/diff/3002/3004#newcode7680
src/ia32/codegen-ia32.cc:7680: __ pop(eax);
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) {
Indentation.
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.
I think there is a zero '0' instead of the letter 'O' here?
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 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) {
Remove space before '*'.
http://codereview.chromium.org/551093
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
