Hairy stuff. :)
First round of comments. Once these are taken care of I would like to take
one
more look.
http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4851
src/hydrogen.cc:4851: // residing in new space. If it is the assumption
used for optimization is
Simplify to: If it is we assume that the function will stay the same?
http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4862
src/hydrogen.cc:4862: Handle<JSGlobalPropertyCell> cell =
Handle<JSGlobalPropertyCell>::null();
Do you need the cell declaration out here? Can't you move it into the if
where you actually fetch the value?
http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4878
src/hydrogen.cc:4878: // If the traget is not null we have found a known
global function that is
traget -> target
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode4977
src/ia32/code-stubs-ia32.cc:4977: // This code can patch an call site
inlined cache of the instance of check,
an call site -> a call site
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode4983
src/ia32/code-stubs-ia32.cc:4983: // address to the cmp instruction just
just below the return address.
just just -> just
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode4985
src/ia32/code-stubs-ia32.cc:4985: // esp[4] : delta from return
address to cmp instruction
You should add to this comment that call site patching only happens when
args are in registers. Begin the generation function with:
ASSERT(HasArgsInRegisters() || !HasCallSiteInlineCheck())
?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5041
src/ia32/code-stubs-ia32.cc:5041: // Update the global instanceof cache
with the current map and function. The
global -> global or inlined?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5050
src/ia32/code-stubs-ia32.cc:5050: // The constatns in for the code
patching are based on no push instructions
constatns in for -> constants for
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5056
src/ia32/code-stubs-ia32.cc:5056: __ add(Operand(scratch),
Immediate(2));
Named constant? Also, can't this be part of the operand in the move
below?
Can you generate code when running with --debug-code that checks that
you are patching a cmp instruction here? Similarly for the moves that
you patch below?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5084
src/ia32/code-stubs-ia32.cc:5084: __ add(Operand(scratch),
Immediate(13));
Named constant? Combine with move?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5103
src/ia32/code-stubs-ia32.cc:5103: __ add(Operand(scratch),
Immediate(13));
Same here.
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5154
src/ia32/code-stubs-ia32.cc:5154:
two blank lines between functions
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5156
src/ia32/code-stubs-ia32.cc:5156:
Two blanks.
http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5179
src/ia32/code-stubs-ia32.cc:5179: "InstanceofStub%s%s",
Isn't there a %s missing here?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia32.cc#newcode1759
src/ia32/lithium-codegen-ia32.cc:1759: // instance off stub.
off -> of
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia32.cc#newcode1803
src/ia32/lithium-codegen-ia32.cc:1803:
Remove extra newline?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia32.cc#newcode1807
src/ia32/lithium-codegen-ia32.cc:1807: // offset to the location where
to patch the map.
to the location of the map check?
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia32.cc#newcode1823
src/ia32/lithium-codegen-ia32.cc:1823: // Put the result value into the
eax slot and restore all registres.
registres -> registers
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (left):
http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-ia32.h#oldcode975
src/ia32/lithium-ia32.h:975:
Accidental edit?
http://codereview.chromium.org/5990005/diff/16001/src/lithium-allocator.h
File src/lithium-allocator.h (right):
http://codereview.chromium.org/5990005/diff/16001/src/lithium-allocator.h#newcode800
src/lithium-allocator.h:800: // Marks the current instruction as a
requiring saving double registers.
as a -> as
http://codereview.chromium.org/5990005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
