Overall, I'm really glad to see this, especially since it doesn't impact performance.  Much cleaner!
Is there a plan to move the cache_impl.hh code to cache.cc now that it's not templated?

src/mem/cache/cache.hh (Diff revision 1)

do we still need this?  seems like a good opportunity to do s/BlkType/CacheBlk/g

Show all issues

1. Andreas Hansson April 1, 2015, 2:06 p.m. (April 1, 2015, 2:06 p.m.)

   That's certainly an option

src/mem/cache/cache_impl.hh (Diff revision 1)

should this comment be above the method?

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 1)

move open brace up to same line as 'for'

Show all issues

src/mem/cache/cache_impl.hh (Diff revision 1)

is 'this->' necessary?

Show all issues

1. Andreas Hansson April 2, 2015, 2:06 a.m. (April 2, 2015, 2:06 a.m.)

   Yes

2. Steve Reinhardt April 2, 2015, 8:33 a.m. (April 2, 2015, 8:33 a.m.)

   I see... I missed that fn was a PTMF

src/mem/cache/cache_impl.hh (Diff revision 1)

'{' placement

Show all issues

src/mem/cache/tags/base_set_assoc.cc (Diff revision 1)

do we really want to replace the visitor pattern with this?  I don't know how often this is used, but building this whole list seems like unnecessary overhead.  I guess what we really need is the C++ equivalent of python generators...

Show all issues

1. Andreas Hansson April 2, 2015, 2:06 a.m. (April 2, 2015, 2:06 a.m.)

   It is only used as part of flushing and checkpointing so it should not be a performance issue. Perhaps leave it as is for now?

2. Steve Reinhardt April 2, 2015, 8:33 a.m. (April 2, 2015, 8:33 a.m.)

   Perhaps... if we were just implementing this for the first time, it would only be a little sad, but the fact that we had a nice callback structure and are replacing it with this inefficient approach makes me extra sad, and the fact that we're still layering a callback interface on top of it just deepens the sorrow :(.
   Wouldn't it be as simple as replacing BaseTags::getBlkList() with
   
       virtual bool visitBlocks(Cache *, VisitorFnPtr fn) = 0;
   then rewrite this function:
   bool
   
   Cache::visitBlocks(VisitorFnPtr fn)
   
   {
   
       return tags->visitBlocks(this, fn);
   
   }
   and for example:
   bool
   
   BaseSetAssoc::visitBlocks(Cache cache, VisitorFnPtr fn)
   
   {
   
       for (...) {
   
           for (...) {
   
               if (!(cache->fn)(sets[i].blks[j]))
   
                   return false;
   
           }
   
       }
   return true;
   
   }

3. Steve Reinhardt April 2, 2015, 8:35 a.m. (April 2, 2015, 8:35 a.m.)

   Sorry about the line wrapping... I hypothesize that I should have un-checked the 'enable markdown' box on that one.  Let's see if that helps:
   
   
   bool
   BaseSetAssoc::visitBlocks(Cache *cache, VisitorFnPtr fn)
   {
       for (...) {
           for (...) {
               if (!(cache->*fn)(sets[i].blks[j]))
                   return false;
           }
       }
   
       return true;
   }

4. Andreas Hansson April 20, 2015, 10:43 a.m. (April 20, 2015, 10:43 a.m.)

   Good idea, I struggle to make this work though, since there is no way of making the tags include cache.hh (without things being circular).
   The only sensible suggestion seems to be a BaseCacheBlkVisitor with a virtual bool operator()(BlkType& blk) =0 that we then expose to the tags visitBlocks, and subclass for the various visitors in the cache.

src/mem/cache/tags/fa_lru.cc (Diff revision 1)

space before '{'

Show all issues

src/mem/cache/cache.hh (Diff revision 2)

should be VisitorFnPtr (since it's a type)

Show all issues

Thanks for reinstating the callbacks!  And also for waiting so patiently for the review!

src/mem/cache/blk.hh (Diff revision 3)

I don't think this needs to be a template anymore, does it?  It's only instantiated once with <Cache> as the parameter.  I suggest getting rid of the template and also the WrappedBlkVisitor typedef below. The new class could use either name.

Show all issues

1. Andreas Hansson April 29, 2015, 10:02 a.m. (April 29, 2015, 10:02 a.m.)

   Not possible since the block cannot include the Cache. The template parameter is really just there to break the circular dependency.

2. Steve Reinhardt April 29, 2015, 12:05 p.m. (April 29, 2015, 12:05 p.m.)

   I see.  But since the definition only uses 'CacheBlk &', there isn't a dependency on the full CacheBlk definition, so I think the circular dependency could be avoided without the template just by moving the definition to cache.hh, right?
   I see where that leads to some follow-on issues, since you need the CacheBlkVisitor base class, but so does CacheBlkIsDirtyVisitor, and as it stands the latter does rely on the CacheBlk definition.  However, there's really no need for CacheBlkIsDirtyVisitor::operator() to be defined in the header... you could move that method (or in fact the whole CacheBlkIsDirtyVisitor class declaration) into cache_impl.hh right above the isDirty() method.  Then you could move both CacheBlkVisitor and the untemplated CacheBlkVisitorWrapper/WrappedBlkVisitor into cache.hh.
   Not trying to make your life difficult, honest... if you want to defer all this to some future patch that's fine with me.  But it would be nice to make it happen, since the template is conceptually unnecessary.

3. Andreas Hansson April 29, 2015, 2:14 p.m. (April 29, 2015, 2:14 p.m.)

   All done. Would be great to get this batch of patches committed asap.
   Thanks for the review btw. It has definitely improved the design.

src/mem/cache/blk.hh (Diff revision 3)

should be VisitorPtr

Show all issues

1. Andreas Hansson April 29, 2015, 10:02 a.m. (April 29, 2015, 10:02 a.m.)

   Will fix.

Thanks!
Wouldn't it make sense to move the CacheBlkVisitor base class to cache.hh too?  Whether you make that change or not, go ahead and ship it... no need for another reviewboard round-trip.