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
