----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1809/#review4234 -----------------------------------------------------------
Thanks for the patch. I have some high-level comments at this point, most important is how to deal with the blocking. configs/common/CacheConfig.py <http://reviews.gem5.org/r/1809/#comment3996> high or low? configs/common/Options.py <http://reviews.gem5.org/r/1809/#comment3997> type? configs/common/Options.py <http://reviews.gem5.org/r/1809/#comment3998> For the address striping in the bus I opted for the high bit. I don't really care too much, but it would be good to keep it consistent. src/mem/cache/base.hh <http://reviews.gem5.org/r/1809/#comment3999> This is rather unfortunate. Please keep as much as possible private/protected src/mem/cache/base.hh <http://reviews.gem5.org/r/1809/#comment4000> const src/mem/cache/base.hh <http://reviews.gem5.org/r/1809/#comment4001> private for the attributes src/mem/cache/base.hh <http://reviews.gem5.org/r/1809/#comment4002> See base/addr_range for an example of the use of the bit operators. src/mem/cache/base.cc <http://reviews.gem5.org/r/1809/#comment4003> I would suggest parentheses to make the binding more clear src/mem/cache/base.cc <http://reviews.gem5.org/r/1809/#comment4004> fatal? src/mem/cache/base.cc <http://reviews.gem5.org/r/1809/#comment4005> const src/mem/cache/base.cc <http://reviews.gem5.org/r/1809/#comment4006> return inService && nextIdleTick <= curTick() src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1809/#comment4008> csprintf used in most other places src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1809/#comment4007> bank.push_back? src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1809/#comment4009> I would suggest iterators. At least use bank.size() for the limit src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1809/#comment4010> Are there other cases here? src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1809/#comment4012> Can we not use the same method for both cases and consider the bank one of the reasons used for blocking (and thus also call unblock once the bank is available)? src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1809/#comment4011> I am not sure I understand this bit. Is the bank blocked until a response comes back? If so, why? Is the idea not to block while the read/write port of the SRAM is busy? - Andreas Hansson On March 31, 2013, 3:48 p.m., Xiangyu Dong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1809/ > ----------------------------------------------------------- > > (Updated March 31, 2013, 3:48 p.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9627:6122d201ff80 > --------------------------- > 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/Caches.py 47591444a7c5 > configs/common/Options.py 47591444a7c5 > src/mem/cache/BaseCache.py 47591444a7c5 > src/mem/cache/SConscript 47591444a7c5 > src/mem/cache/base.hh 47591444a7c5 > src/mem/cache/base.cc 47591444a7c5 > src/mem/cache/cache_impl.hh 47591444a7c5 > configs/common/CacheConfig.py 47591444a7c5 > > 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
