----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7787 -----------------------------------------------------------
Thanks!!! This looks great. I really appreciate all your effort and your flexibility. I'm totally happy with all the code changes; this review is just wordsmithing (and spell checking :) ) on the comments... src/mem/cache/cache.cc (line 673) <http://reviews.gem5.org/r/3254/#comment6748> upstream src/mem/cache/cache.cc (line 1794) <http://reviews.gem5.org/r/3254/#comment6749> sharers src/mem/cache/cache.cc (line 2009) <http://reviews.gem5.org/r/3254/#comment6750> unnecessary src/mem/cache/cache.cc (line 2017) <http://reviews.gem5.org/r/3254/#comment6751> invalidate src/mem/cache/cache.cc (line 2137) <http://reviews.gem5.org/r/3254/#comment6752> the scope of the "not" is a little ambiguous... I'd say "and should end up in the Shared state" to be clear src/mem/cache/cache.cc (line 2374) <http://reviews.gem5.org/r/3254/#comment6753> for src/mem/cache/mshr.cc (line 401) <http://reviews.gem5.org/r/3254/#comment6754> is "dubious" the right term? It's correct to say that we know we'll end up with a dirty line here. It's just incorrect to assume that, if we're not here, that means we won't get a writable copy, but I think that's irrelevant. src/mem/cache/mshr.cc (line 469) <http://reviews.gem5.org/r/3254/#comment6755> "a writable" src/mem/packet.hh (line 281) <http://reviews.gem5.org/r/3254/#comment6745> I don't believe we ever pass dirty unless the block is becoming writable as well. That is, on a non-needsExclusive request that hits an O (not M) block, cacheResponding() will be set but dirty will not be passed. In general, it seems redundant to have comments here on the constants as well as on the accessors, which potentially leads to confusion when the comments aren't consistent. It might be better to have just single-phrase descriptions here with pointers to the accessors rather than trying to be descriptive in both places. src/mem/packet.hh (line 536) <http://reviews.gem5.org/r/3254/#comment6746> I think this comment (staring at "Note that...") is a little confusing, esp. in light of the following paragraph. Maybe start with what happens when hasSharers is set, then say that if hasSharers is not set, then the responder is passing the dirty copy, and because the responder either had the block writable to begin with or all other copies have been invalidated, the requester always ends up with a Modified rather than Owned copy. src/mem/packet.hh (line 601) <http://reviews.gem5.org/r/3254/#comment6747> I'd say "on responding to a snooped request"... "providing a snoop response" sounds like you could just be talking about setting the hasSharers() bit. - Steve Reinhardt On Dec. 29, 2015, 7:13 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3254/ > ----------------------------------------------------------- > > (Updated Dec. 29, 2015, 7:13 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11281:1f9291ca37dd > --------------------------- > mem: Make cache terminology easier to understand > > This patch changes the name of a bunch of packet flags and MSHR member > functions and variables to make the coherency protocol easier to > understand. In addition the patch adds and updates lots of > descriptions, explicitly spelling out assumptions. > > The following name changes are made: > > * the packet memInhibit flag is renamed to cacheResponding > > * the packet sharedAsserted flag is renamed to hasSharers > > * the packet NeedsExclusive attribute is renamed to NeedsWritable > > * the packet isSupplyExclusive is renamed responderHadWritable > > * the MSHR pendingDirty is renamed to pendingModified > > The cache states, Modified, Owned, Exclusive, Shared are also called > out in the cache and MSHR code to make it easier to understand. > > > Diffs > ----- > > src/dev/dma_device.cc 3fd1142adad9 > src/mem/abstract_mem.cc 3fd1142adad9 > src/mem/addr_mapper.cc 3fd1142adad9 > src/mem/bridge.cc 3fd1142adad9 > src/mem/cache/base.hh 3fd1142adad9 > src/mem/cache/blk.hh 3fd1142adad9 > src/mem/cache/cache.hh 3fd1142adad9 > src/mem/cache/cache.cc 3fd1142adad9 > src/mem/cache/mshr.hh 3fd1142adad9 > src/mem/cache/mshr.cc 3fd1142adad9 > src/mem/cache/mshr_queue.hh 3fd1142adad9 > src/mem/cache/mshr_queue.cc 3fd1142adad9 > src/mem/coherent_xbar.cc 3fd1142adad9 > src/mem/comm_monitor.cc 3fd1142adad9 > src/mem/dram_ctrl.cc 3fd1142adad9 > src/mem/dramsim2.cc 3fd1142adad9 > src/mem/hmc_controller.cc 3fd1142adad9 > src/mem/mem_checker_monitor.cc 3fd1142adad9 > src/mem/noncoherent_xbar.cc 3fd1142adad9 > src/mem/packet.hh 3fd1142adad9 > src/mem/packet.cc 3fd1142adad9 > src/mem/ruby/system/DMASequencer.cc 3fd1142adad9 > src/mem/ruby/system/RubyPort.cc 3fd1142adad9 > src/mem/serial_link.cc 3fd1142adad9 > src/mem/simple_mem.cc 3fd1142adad9 > src/mem/snoop_filter.cc 3fd1142adad9 > src/mem/tport.cc 3fd1142adad9 > src/cpu/o3/cpu.cc 3fd1142adad9 > > Diff: http://reviews.gem5.org/r/3254/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
