Yes, I can return from the loop. But it makes the entire loop unroll. I'm not sure this is a good idea. Also, unless we return from inside the loop, returning -1 (no_reg.code()) instead of kNumRegisters just adds an extra test, since when we get the return value, we immediately compare it to kNumRegisters (or -1) immediately anyway. This function is just called from one site, and its purpose is to generate a tight inlined loop at that site.
On Wed, Mar 18, 2009 at 6:51 AM, Kasper Lund <[email protected]> wrote: > My only comments are that I'd like to get rid of the break in the loop > in ScanForFreeRegister (can't you just return?) and that returning > kNumRegisters as an indication that no free registers exist seems > plain weird. > > Cheers, > Kasper > > On Tue, Mar 17, 2009 at 12:36 PM, <[email protected]> wrote: > > Reviewers: Kasper Lund, Kevin Millikin, > > > > Message: > > A tiny code review for you. > > The inner loop could be made > > > > while (ref_counts_[++i]); > > > > with a sentinel, but I think it is fast enough as is. > > > > Description: > > Speed up the inner loop of free register allocation. > > > > Please review this at http://codereview.chromium.org/42296 > > > > SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ > > > > Affected files: > > M src/register-allocator.h > > M src/register-allocator.cc > > > > > > Index: src/register-allocator.h > > =================================================================== > > --- src/register-allocator.h (revision 1525) > > +++ src/register-allocator.h (working copy) > > @@ -149,6 +149,7 @@ > > // Record that a register will no longer be used by decrementing its > > // reference count. > > void Unuse(Register reg) { > > + ASSERT(!reg.is(no_reg)); > > ASSERT(is_used(reg.code())); > > if (is_used(reg.code())) { > > ref_counts_[reg.code()]--; > > @@ -161,6 +162,15 @@ > > private: > > int ref_counts_[kNumRegisters]; > > > > + // Very fast inlined loop to find a free register. > > + inline int ScanForFreeRegister() { > > + int i = 0; > > + for (; i < kNumRegisters ; ++i ) { > > + if (ref_counts_[i] == 0) break; > > + } > > + return i; > > + } > > + > > friend class RegisterAllocator; > > }; > > > > Index: src/register-allocator.cc > > =================================================================== > > --- src/register-allocator.cc (revision 1525) > > +++ src/register-allocator.cc (working copy) > > @@ -83,11 +83,10 @@ > > > > Result RegisterAllocator::AllocateWithoutSpilling() { > > // Return the first free register, if any. > > - for (int i = 0; i < kNumRegisters; i++) { > > - if (!is_used(i)) { > > - Register free_reg = { i }; > > - return Result(free_reg, cgen_); > > - } > > + int free_reg = registers_.ScanForFreeRegister(); > > + if (free_reg < kNumRegisters) { > > + Register free_result = { free_reg }; > > + return Result(free_result, cgen_); > > } > > return Result(cgen_); > > } > > > > > > > -- We can IMAGINE what is not --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
