> On July 31, 2015, 11:13 p.m., Joel Hestness wrote: > > Thanks for updating this and apologies for the delayed review. > > > > Besides the comments below, this looks a lot more agreeable.
We are just hours away from checking our code in, so we won't be able to address your comments. We did put a lot effort into adding the coalesce_reqs flag as you requested in your last review. > On July 31, 2015, 11:13 p.m., Joel Hestness wrote: > > src/mem/ruby/system/Sequencer.cc, line 258 > > <http://reviews.gem5.org/r/2787/diff/3/?file=48362#file48362line258> > > > > Can you adjust this comment to be consistent with the check (i.e. > > 'check for any outstanding requests to the line')? We'll have to get it next time we check in patches. Or you're welcome to update yourself. > On July 31, 2015, 11:13 p.m., Joel Hestness wrote: > > src/mem/ruby/system/Sequencer.cc, line 553 > > <http://reviews.gem5.org/r/2787/diff/3/?file=48362#file48362line553> > > > > I'm still not convinced that this should be allowed, since this assumes > > the L1 cache is RFO. My preference is that the read request be issued to > > the caches instead (mirroring the readCallback). If the caches are RFO, > > then issuing reads here should complete in the next cycle, so this > > shouldn't affect performance much. There is a lot of code in the Sequencer that assumes RfO (example the block on calls to support RMW). - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2787/#review6875 ----------------------------------------------------------- On July 22, 2015, 6:15 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2787/ > ----------------------------------------------------------- > > (Updated July 22, 2015, 6:15 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10887:1e05089bc991 > --------------------------- > ruby: Fixed pipeline squashes caused by aliased requests > > This patch was created by Bihn Pham during his internship at AMD. > > This patch fixes a very significant performance bug when using the O3 CPU > model > and Ruby. The issue was Ruby returned false when it received a request to the > same address that already has an outstanding request or when the memory is > blocked. As a result, O3 unnecessary squashed the pipeline and re-executed > instructions. This fix merges readRequestTable and writeRequestTable in > Sequencer into a single request table that keeps track of all requests and > allows multiple outstanding requests to the same address. This prevents O3 > from squashing the pipeline. > > > Diffs > ----- > > src/mem/ruby/system/Sequencer.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/Sequencer.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/Sequencer.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > > Diff: http://reviews.gem5.org/r/2787/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
