Thanks a lot, Erik. yours, anton.
On Tue, Feb 2, 2010 at 1:23 PM, Erik Corry <[email protected]> wrote: > 2010/1/29 <[email protected]>: >> 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? > > It is a strange convention. The motivation is that it visually > unclutters the code. At this stage it's mainly a question of keeping > a consistent style. Also, if I ever get the generated code coverage > going again then it will be important. The generated code coverage > build looks for lines containing this pattern and checks that we > execute some code generated by that line. That's subtly different > from asserting that the C++ line gets executed since we could execute > it, but always branch past the generated code. > >> >> 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? > > Sounds good. > >> >>> 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
