Thanks for comments, Erik.

That was good idea to check I take all the branches, apparently I didn't go into
CallIC and some LoadIC---hopefully that could improve performance.


http://codereview.chromium.org/551191/diff/2001/3001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/551191/diff/2001/3001#newcode495
src/arm/stub-cache-arm.cc:495: masm->tst(r1, Operand(kSmiTagMask));
On 2010/01/28 20:43:13, Erik Corry wrote:
Please use __ instead of masm->

Done.  Just for my education, why it's preferred other explicit
construction?

And how would you feel if I move (in a later CL) this code into macro
assembler itself (for all platforms)?  Would it be to much?

The Macro assembler has the BranchOnSmi call for this pattern.  It's
not used
consistently, but I think it should be used for new code.

Cool.  I was looking for JumpOnSmi, too intel'ish I was.

http://codereview.chromium.org/551191/diff/2001/3001#newcode661
src/arm/stub-cache-arm.cc:661: masm->cmp(r0,
Operand(Factory::no_interceptor_result_sentinel()));
On 2010/01/28 20:43:13, Erik Corry wrote:
This will load the sentinel from a PC-relative address, then emit
reloc info so
the GC understands.  Then it will do a register-register cmp.  Instead
it is
more compact to use LoadRoot to load into a scratch register, then do
the cmp
explicitly.  It's OK to use ip as the scratch register (that is what
the cmp
will do).

Thanks.  Done.

http://codereview.chromium.org/551191/diff/2001/3001#newcode791
src/arm/stub-cache-arm.cc:791: masm->cmp(r0,
Operand(Factory::no_interceptor_result_sentinel()));
On 2010/01/28 20:43:13, Erik Corry wrote:
LoadRoot

Done.

http://codereview.chromium.org/551191/diff/2001/3001#newcode803
src/arm/stub-cache-arm.cc:803: // TODO(antonm): get rid of argc_
On 2010/01/28 20:43:13, Erik Corry wrote:
No commented out code.

Sorry, remnant of moving.  Removed.

  What will it take to get rid of argc?  Instead of a TODO
it is preferable to file a bug unless you are going to fix this
tomorrow.

Yes, I am planning to do it tomorrow/today :)

http://codereview.chromium.org/551191/diff/2001/3002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/551191/diff/2001/3002#newcode550
src/ia32/stub-cache-ia32.cc:550: Handle<Code> code(function->code());
On 2010/01/28 20:43:13, Erik Corry wrote:
Why don't we want to assert this any more?

It's checked 6 lines above.

http://codereview.chromium.org/551191

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to