I have addressed most of the comments. Still not sure what to do with
BinaryOpIC::patch. Also uploaded a rebased patch.
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
On 2010/02/18 10:31:34, Kevin Millikin wrote:
UNKNOWN ==> DEFAULT
Done.
http://codereview.chromium.org/553117/diff/13001/14002#newcode8005
src/ia32/codegen-ia32.cc:8005: GenerateRegisterArgsPush(masm);
On 2010/02/18 10:31:34, Kevin Millikin wrote:
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.
Done.
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());
On 2010/02/18 10:31:34, Kevin Millikin wrote:
Should be no space between Code and *.
Done.
http://codereview.chromium.org/553117/diff/13001/14004#newcode1451
src/ic.cc:1451: // if (left->IsString() || right->IsString()) {
On 2010/02/18 10:31:34, Kevin Millikin wrote:
Don't commit commented-out code.
Done.
http://codereview.chromium.org/553117/diff/13001/14004#newcode1466
src/ic.cc:1466: NoHandleAllocation na;
On 2010/02/18 10:31:34, Kevin Millikin wrote:
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.
Done.
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,
On 2010/02/18 10:31:34, Kevin Millikin wrote:
The distinction between DEFAULT and GENERIC is not obvious. Maybe a
comment
will help.
Added a comment.
http://codereview.chromium.org/553117/diff/13001/14006#newcode458
src/ic.h:458: void patch(Code* code);
On 2010/02/18 10:31:34, Kevin Millikin wrote:
Is this function needed?
The only reason for its existence is that IC::set_target is protected. I
could either merge this function into BinaryOpIC constructor or make
IC::set_target public but I am not sure I like any of the two options.
http://codereview.chromium.org/553117
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
