It looks much cleaner now.
There is a GC issue in BinarOp_Patch that needs to be fixed, but the rest
looks
like a good start.
http://codereview.chromium.org/553117/diff/13001/14015
File src/code-stubs.h (right):
http://codereview.chromium.org/553117/diff/13001/14015#newcode144
src/code-stubs.h:144: virtual int GetCodeKind();
As we discussed, after this change lands we should try to see if the
binary op stubs can be made into ICs (inheriting from class IC).
For example, it's strange that a code stub reports a kind XXX_IC instead
of STUB.
http://codereview.chromium.org/553117/diff/13001/14002
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/553117/diff/13001/14002#newcode7909
src/ia32/codegen-ia32.cc:7909: // Generate an unreachable reference to
the UNKNOWN stub so that it can be
UNKNOWN ==> DEFAULT
http://codereview.chromium.org/553117/diff/13001/14002#newcode7911
src/ia32/codegen-ia32.cc:7911: if (runtime_operands_type_ !=
BinaryOpIC::DEFAULT) {
As we discussed, we should (a) evaluate whether we want to clear these
or not and (b) if so find a more elegant way to encode the default stub.
http://codereview.chromium.org/553117/diff/13001/14002#newcode8005
src/ia32/codegen-ia32.cc:8005: GenerateRegisterArgsPush(masm);
I find it a little confusing that GenerateRegisterArgsPush doesn't
generate anything if !HasArgsInRegisters() and GenerateLoadArguments
doesn't generate anything if HasArgsInRegisters.
I'd like it better if it was assumed (ASSERTed) that HasArgsInRegisters
for GenerateRegisterArgsPush and !HasArgsInRegisters for
GenerateLoadArguments. Then the code here becomes:
// Keep a copy of operands on the stack and make sure they are also in
edx,eax.
if (HasArgsInRegister()) {
GenerateRegisterArgsPush(masm);
} else {
GenerateLoadArguments(masm);
}
And all the other call sites changed similarly.
http://codereview.chromium.org/553117/diff/13001/14004
File src/ic.cc (right):
http://codereview.chromium.org/553117/diff/13001/14004#newcode1409
src/ic.cc:1409: Code * uninit_stub =
Code::GetCodeFromTargetAddress(rinfo->target_address());
Should be no space between Code and *.
http://codereview.chromium.org/553117/diff/13001/14004#newcode1451
src/ic.cc:1451: // if (left->IsString() || right->IsString()) {
Don't commit commented-out code.
http://codereview.chromium.org/553117/diff/13001/14004#newcode1466
src/ic.cc:1466: NoHandleAllocation na;
This function looks risky for GC. The call to GetBinaryOpStub will call
GetCode, which may have to compile the stub and allocate, which may
trigger a GC.
If that happens, the raw Object pointers will not be safe to use
(specifically, result).
Solution is to put left, right, result in handles:
Handle<Object> left = args.at<Object>(0);
Handle<Object> right = args.at<Object>(1);
Then put a HandleScope in this function around the call to
GetBinaryOpStub and the code that uses its result. Remove the
HandleScope from GetBinaryOpStub and make it return Handle<Code>.
There's an example of the style in this file in CallIC_Miss.
http://codereview.chromium.org/553117/diff/13001/14006
File src/ic.h (right):
http://codereview.chromium.org/553117/diff/13001/14006#newcode450
src/ic.h:450: DEFAULT,
The distinction between DEFAULT and GENERIC is not obvious. Maybe a
comment will help.
http://codereview.chromium.org/553117/diff/13001/14006#newcode458
src/ic.h:458: void patch(Code* code);
Is this function needed?
http://codereview.chromium.org/553117
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
