> On March 4, 2016, 9:30 a.m., Steve Reinhardt wrote: > > Seems fine, though: > > 1. It's weird that we're passing in a 1 to mean "no reserve entries" and > > "p->mshrs + 1" to mean "as may reserve entries as MSHRs". I realize that's > > an issue with the semantics of the parameter, and outside the scope of this > > patch, but if we're ever going to clean that up now would be a good time. > > 2. it would be nice to have a forward pointer comment by the initializers > > so people know there's a more detailed comment below > > Andreas Hansson wrote: > 1. Is addressed. I completely agree with you. > 2. We don't use comments interspersed with the initialisation list > anywhere else in the codebase, and I'm really not a fan. What did you have in > mind?
Thanks for fixing #1. As far as #2, I wasn't suggesting anything wild or radical (or specific), just pointing out that if you're looking at those initializer lines and wondering where the argument values came from, you may never know that there's a comment explaining it 16 lines below. Maybe just a "// see below" at the end of each line? (And vice versa, it would be helpful to specifically mention mshrQueue and writeBuffer in the comment, just to make it crystal clear that those are the variables you're referring to.) - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3347/#review8056 ----------------------------------------------------------- On March 6, 2016, 9:38 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3347/ > ----------------------------------------------------------- > > (Updated March 6, 2016, 9:38 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11367:5fe50a8b3c55 > --------------------------- > mem: Adjust cache queue reserve to more conservative values > > The cache queue reserve is there as an overflow to give us enough > headroom based on when we block the cache, and how many transactions > we may already have accepted before actually blocking. The previous > values were probably chosen to be "big enough", when we actually know > that we check the MSHRs after every single allocation, and for the > write buffers we know that we implicitly may need one entry for every > outstanding MSHR. > > > Diffs > ----- > > src/mem/cache/base.cc 1bd9f1b27438 > src/mem/cache/queue.hh PRE-CREATION > > Diff: http://reviews.gem5.org/r/3347/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
