----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2788/#review6263 -----------------------------------------------------------
src/cpu/o3/lsq_unit.hh (line 917) <http://reviews.gem5.org/r/2788/#comment5400> I am not a huge fan of the massive conditional and the code flow. First simple change is to check for the inverse and return NoFault early. Is there any chance to annotate the reasoning behind some of these? src/cpu/o3/lsq_unit_impl.hh (line 1242) <http://reviews.gem5.org/r/2788/#comment5401> What is the point of factoring this out into a separate function? I'd think most of the logic in LSQUnit::write could / should be moved into this function. Wrapping the dcachePort->sendTimingReq really does not make any difference. src/mem/abstract_mem.cc (line 382) <http://reviews.gem5.org/r/2788/#comment5402> Why is the store access not an invalidate? src/mem/cache/cache_impl.hh (line 328) <http://reviews.gem5.org/r/2788/#comment5408> Superfluous debug? Should be handled by 302 already. src/mem/cache/cache_impl.hh (line 394) <http://reviews.gem5.org/r/2788/#comment5409> Drop. src/mem/cache/cache_impl.hh (line 582) <http://reviews.gem5.org/r/2788/#comment5410> Unneccessary fast-out (althrough I would like the fast-path to be more obvious in gernal in this function). Also.. who deallocates the pkt? src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/2788/#comment5411> Please don't remove these assertions. Add a more specific one with a justification. src/mem/cache/mshr.cc (line 218) <http://reviews.gem5.org/r/2788/#comment5412> Surperfluous (..) around isHWPrefetch vs quite (too IMHO) sparing usage of () in other introduced conditionals. src/mem/cache/prefetch/queued.cc (line 68) <http://reviews.gem5.org/r/2788/#comment5413> pull out blk_addr computation src/mem/packet.hh (line 124) <http://reviews.gem5.org/r/2788/#comment5407> Why add both and not start out with sending HardPFExReq straight away? src/mem/packet.hh (line 149) <http://reviews.gem5.org/r/2788/#comment5403> Drop, if not used anywhere except redundant check which could be isInvalidate. src/mem/packet.hh (line 704) <http://reviews.gem5.org/r/2788/#comment5405> The comment is confusing; the "hence" part. Why isn't the HardPFexReq marked as NeedsResponse, too (similar to HardPFReq)? src/mem/packet.cc (line 96) <http://reviews.gem5.org/r/2788/#comment5406> See earlier comment regarding NeedsResponse? - Stephan Diestelhorst On May 11, 2015, 10:22 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2788/ > ----------------------------------------------------------- > > (Updated May 11, 2015, 10:22 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10845:0aa7c6802baa > --------------------------- > cpu: Add store-access operations > > This patch was created by Bihn Pham during his internship at AMD. > > This patch adds decoupled store operations that are commonly used by > high-performance processors. The patch modifies the O3 CPU model to issue > StoreAccess operations as soon as possible to acquire exclusive permission for > eventual stores. Later data store operations behave as normal. > > In the classic memory model, StoreAccess requests are treated as HardPFExReq > and share the existing prefetch queue. Therefore, they affect performance only > if the prefetcher is enabled in the classic model. > > In Ruby, StoreAccess requests are treated as RubyStore with NULL data field. > > > Diffs > ----- > > src/cpu/o3/lsq_unit.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/cpu/o3/lsq_unit_impl.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/abstract_mem.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/cache/cache_impl.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/cache/mshr.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/cache/prefetch/queued.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/packet.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/packet.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/protocol/RubySlicc_Exports.sm > fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/system/RubyPort.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/system/RubyPort.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/ruby/system/Sequencer.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > > Diff: http://reviews.gem5.org/r/2788/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
