Addressed all the comments. Also changed the call site cache to use a near label, saving 4 bytes.
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 On 2011/01/04 17:25:58, Mads Ager wrote:
Simplify to: If it is we assume that the function will stay the same?
Done. http://codereview.chromium.org/5990005/diff/16001/src/hydrogen.cc#newcode4862 src/hydrogen.cc:4862: Handle<JSGlobalPropertyCell> cell = Handle<JSGlobalPropertyCell>::null(); On 2011/01/04 17:25:58, Mads Ager wrote:
Do you need the cell declaration out here? Can't you move it into the
if where
you actually fetch the value?
Don't need the cell at all, can just use lookup.GetValue. 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 On 2011/01/04 17:25:58, Mads Ager wrote:
traget -> target
Done. 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, On 2011/01/04 17:25:58, Mads Ager wrote:
an call site -> a call site
Done. 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. On 2011/01/04 17:25:58, Mads Ager wrote:
just just -> just
Done. 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 On 2011/01/04 17:25:58, Mads Ager wrote:
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())
?
Done. 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 On 2011/01/04 17:25:58, Mads Ager wrote:
global -> global or inlined?
Done. 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 On 2011/01/04 17:25:58, Mads Ager wrote:
constatns in for -> constants for
Done. 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)); On 2011/01/04 17:25:58, Mads Ager wrote:
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? Done. 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)); On 2011/01/04 17:25:58, Mads Ager wrote:
Named constant? Combine with move?
Done. 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)); On 2011/01/04 17:25:58, Mads Ager wrote:
Same here.
Done. http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5154 src/ia32/code-stubs-ia32.cc:5154: On 2011/01/04 17:25:58, Mads Ager wrote:
two blank lines between functions
Done. http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5156 src/ia32/code-stubs-ia32.cc:5156: On 2011/01/04 17:25:58, Mads Ager wrote:
Two blanks.
Done. http://codereview.chromium.org/5990005/diff/16001/src/ia32/code-stubs-ia32.cc#newcode5179 src/ia32/code-stubs-ia32.cc:5179: "InstanceofStub%s%s", On 2011/01/04 17:25:58, Mads Ager wrote:
Isn't there a %s missing here?
Done. 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. On 2011/01/04 17:25:58, Mads Ager wrote:
off -> of
Done. http://codereview.chromium.org/5990005/diff/16001/src/ia32/lithium-codegen-ia32.cc#newcode1803 src/ia32/lithium-codegen-ia32.cc:1803: On 2011/01/04 17:25:58, Mads Ager wrote:
Remove extra newline?
Done. 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. On 2011/01/04 17:25:58, Mads Ager wrote:
to the location of the map check?
Done. 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. On 2011/01/04 17:25:58, Mads Ager wrote:
registres -> registers
Done. 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: On 2011/01/04 17:25:58, Mads Ager wrote:
Accidental edit?
Done. 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. On 2011/01/04 17:25:58, Mads Ager wrote:
as a -> as
Done. http://codereview.chromium.org/5990005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
