> On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > configs/common/Caches.py, line 54 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36785#file36785line54> > > > > Could we not let num_banks be 1 by default and leave this out here? > > > > Similary so for the interleaving bit. Make it 0 by default and if > > num_banks is 1 then it is never needed.
I think I have some confusion here. If the command-line never explicitly specify a parameter value, where is the standard place we set the default value in the code? In BaseCache.py? > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > configs/common/Options.py, line 107 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36787#file36787line107> > > > > Could you add "(cycles)" at the end? added > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > configs/common/Options.py, line 124 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36787#file36787line124> > > > > I am not sure it's worth removing this functionality, but would we ever > > want to interleave on anything besides cache line granularity? I will set the default to 0, and 0 means "automatically" set the interleaving bit to cache line granularity. The user can override this decision by setting this parameter. > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > src/mem/cache/base.cc, line 81 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36791#file36791line81> > > > > I would suggest p->bank_intlv_high_bit ? p->bank_intlv_high_bit : > > ceilLog2(blkSize) + bankIntlvBits I see. But, it should be "ceilLog2(blkSize) + bankIntlvBits - 1", right? > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 308 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36792#file36792line308> > > > > else? fatal? lat = readLatency? > > > > I would think if pkt->isWrite(), lat = writeLatency, and in all other > > cases lat = readLatency. Here is the part I really want to discuss with you more. What does &lat returned from tags->accessBlock() mean? I can see most of the time it returns a readLatency, but in some cases, cache->ticksToCycles(blk->whenReady - curTick()) is returned. I'm not sure how it exactly works. My understanding is: the "lat" returned from tags->accessBlock() should be the "tag lookup latency", and depending on whether we access tag and data in parallel or not, the final latency should be max(lat, read(write)Latency) or lat+read(write)Latency. > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 1791 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36792#file36792line1791> > > > > I would really consider this a todo and a next follow-on patch add a "@todo" here about the future event-driven implementation > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 1812 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36792#file36792line1812> > > > > Could it be extended? As you point out, this could happen. extendService() function is a workaround in case we cannot block incoming traffic from memory side during bank busy. We need to apply another patch (#1826) to block the cache installation (from memory side) when bank is busy, and after that, extendService() function can be removed. > On July 31, 2013, 10:17 p.m., Andreas Hansson wrote: > > src/mem/cache/base.cc, line 141 > > <http://reviews.gem5.org/r/1809/diff/12/?file=36791#file36791line141> > > > > The name is a bit misleading, is it not? Combine this function with clearInService(), and rename it to checkAndUnmarkInService() - Xiangyu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1809/#review4560 ----------------------------------------------------------- On July 31, 2013, 9:52 p.m., Xiangyu Dong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1809/ > ----------------------------------------------------------- > > (Updated July 31, 2013, 9:52 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 9818:d6c890fd0eab > --------------------------- > 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 2492d7ccda7e > configs/common/Caches.py 2492d7ccda7e > configs/common/O3_ARM_v7a.py 2492d7ccda7e > configs/common/Options.py 2492d7ccda7e > src/mem/cache/BaseCache.py 2492d7ccda7e > src/mem/cache/SConscript 2492d7ccda7e > src/mem/cache/base.hh 2492d7ccda7e > src/mem/cache/base.cc 2492d7ccda7e > src/mem/cache/cache_impl.hh 2492d7ccda7e > src/mem/cache/tags/Tags.py 2492d7ccda7e > > 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
