> On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/BaseCache.py, line 55 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36723#file36723line55> > > > > Is there a point in having these defaults if the command-line options > > override then anyways? > > > > Int -> Unsigned?
I see. You prefer to set the default in Caches.py instead of here, right? If so, I can move these default settings to Caches.py I checked other parameters, like mshrs, write_buffers. They all use Int. So, I also use Int to make them look consistent. > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/BaseCache.py, line 56 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36723#file36723line56> > > > > Unsigned Same above > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/base.hh, line 526 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36725#file36725line526> > > > > Already do this once in the constructor. made bankIntlvBits a instance variable > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/base.hh, line 292 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36725#file36725line292> > > > > I'd say do the rename as part of this patch. > > > > Anyone else got opinions on this one? I've renamed all hitLatency to readLatency (except for tag and ruby model). > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/base.hh, line 527 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36725#file36725line527> > > > > I would suggest to precompute the mask and shift in the constructor as > > I imagine this will be called a lot. > > > > Any idea what this patch does to the simulator performance btw? add bankIntlvLowBit and bankIntlvMask const variable. and use them to do the operation. > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 1802 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36727#file36727line1802> > > > > This is fine, just wanted to point out that you can also do: > > > > for (auto b = cache->bank.begin(); b != cache->bank.end(); ++b) > > if ((*b)->serviceDone()) etc > > > > The unfortunate bit here is that we iterate over all the banks for > > every cache access. I don't know what the performance impact is, but it > > worries me a bit. > > > > The benefit of having events per bank is that they only do what they > > have to, and only when they have to. I think usually bank number won't beyond 8 or 16. In most cases, I expect it to be 2 or 4. So, the performance impact shall be negligible. But, I agree that these codes should eventually be converted to event-driven. Shall we delay the bank event queue to a later patch? I'm afraid it might be too much for this patch. > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 1812 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36727#file36727line1812> > > > > if (blocked) ? you are right. they are equivalent. > On July 19, 2013, 8:54 a.m., Andreas Hansson wrote: > > configs/common/Options.py, line 106 > > <http://reviews.gem5.org/r/1809/diff/10/?file=36722#file36722line106> > > > > I'm tempted to say we group these based on L1, L2 and L3. > > > > Any other opinions on this one? I see. But, should we have some mechanisms to let the command-line override the default setting in Caches.py? - Xiangyu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1809/#review4536 ----------------------------------------------------------- On July 18, 2013, 7:33 p.m., Xiangyu Dong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1809/ > ----------------------------------------------------------- > > (Updated July 18, 2013, 7:33 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 9817:fc242bb34728 > --------------------------- > mem: model data array bank in classic cache > The classic cache does not model data array bank, i.e. if a read/write is > being > serviced by a cache bank, no other requests should be sent to this bank. > This patch models a multi-bank cache. Features include: > 1. detect if the bank interleave granularity is larger than cache line size > 2. add CacheBank debug flag > 3. Differentiate read and write latency > 3a. read latency is still named as hit_latency > 3b. write latency is named as write_latency > 4. Add write_latency, num_banks, bank_itlv_bit into the Python parser > Not modeled in this patch: > Due to the lack of retry mechanism in the cache master port, the access form > the memory side will not be denied if the bank is in service. Instead, the > bank > service time will be extended. This is equivalent to an infinite write buffer > for cache fill operations. > > > Diffs > ----- > > configs/common/CacheConfig.py 3b3b94536547 > configs/common/Caches.py 3b3b94536547 > configs/common/Options.py 3b3b94536547 > src/mem/cache/BaseCache.py 3b3b94536547 > src/mem/cache/SConscript 3b3b94536547 > src/mem/cache/base.hh 3b3b94536547 > src/mem/cache/base.cc 3b3b94536547 > src/mem/cache/cache_impl.hh 3b3b94536547 > > Diff: http://reviews.gem5.org/r/1809/diff/ > > > Testing > ------- > > > Thanks, > > Xiangyu Dong > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
