----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/704/#review1291 -----------------------------------------------------------
I apologize it took me a while to review this code. Overall, my major concern is that the patch removes some of the most valuable stats in Ruby. Specific comments below: src/mem/ruby/profiler/Profiler.hh <http://reviews.m5sim.org/r/704/#comment1754> Please, do not delete these stats. These stats are very useful in discovering performance bottlenecks in protocols. As far as those protocols checked into the tree, I believe the hammer protocol uses most, if not all of these stats. The term "wCC" stands for "with Cache Coherence" and signifies those requests that require cache-to-cache transfers. src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1755> This code was used to monitor cache port and bank contention when Ruby modeled those resources. I'm pretty sure that no current protocols enable the bank resource feature, but if the port resource is approximated by the number of transitions per cycle. It is possible for that port resource to cause problems and this stat would catch it. Thus I would like to keep these stats. src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1756> Yes, Korey your last observation is exactly why those histograms are initialized with a fixed bin size. It makes comparisons between different runs possible. I assume you can do the same thing with M5 stats, correct? src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1757> Did you move this logic somewhere else, or did you determine that none of the checked in protocols use this function? src/mem/ruby/profiler/Profiler.cc <http://reviews.m5sim.org/r/704/#comment1758> Please keep this funcitonality. It is very useful to understand the behavior of the system. - Brad On 2011-05-16 15:06:16, Derek Hower wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/704/ > ----------------------------------------------------------- > > (Updated 2011-05-16 15:06:16) > > > Review request for Default, Nathan Binkert, Korey Sewell, and Brad Beckmann. > > > Summary > ------- > > This patch contains changes to convert Ruby's stat handling to the M5-style > Stat class. The ultimate goal is to remove Profiler entirely, though this > patch only represents a (significant) step towards that goal. Some stats > remain in the Profiler, notably those that do not have an obvious object to > hold them (like Address Profiler) or those that I'm not sure what they do > (e.g., *wCC*). Also, this patch does not contain a Garnet stats conversion > (though the simple network is converted). > > > Diffs > ----- > > src/mem/ruby/network/simple/SimpleNetwork.hh UNKNOWN > src/mem/ruby/network/simple/SimpleNetwork.cc UNKNOWN > src/mem/ruby/network/simple/Throttle.hh UNKNOWN > src/mem/ruby/network/simple/Throttle.cc UNKNOWN > src/mem/ruby/profiler/AddressProfiler.hh UNKNOWN > src/mem/ruby/profiler/AddressProfiler.cc UNKNOWN > src/mem/ruby/profiler/CacheProfiler.hh UNKNOWN > src/mem/ruby/profiler/CacheProfiler.cc UNKNOWN > src/mem/ruby/profiler/MemCntrlProfiler.hh UNKNOWN > src/mem/ruby/profiler/MemCntrlProfiler.cc UNKNOWN > src/mem/ruby/profiler/Profiler.hh UNKNOWN > src/mem/ruby/profiler/Profiler.cc UNKNOWN > src/mem/ruby/slicc_interface/RubySlicc_Profiler_interface.hh UNKNOWN > src/mem/ruby/slicc_interface/RubySlicc_Profiler_interface.cc UNKNOWN > src/mem/ruby/system/CacheMemory.hh UNKNOWN > src/mem/ruby/system/MemoryControl.hh UNKNOWN > src/mem/ruby/system/Sequencer.hh UNKNOWN > src/mem/ruby/system/Sequencer.cc UNKNOWN > src/mem/ruby/system/System.cc UNKNOWN > src/mem/slicc/symbols/StateMachine.py UNKNOWN > > Diff: http://reviews.m5sim.org/r/704/diff > > > Testing > ------- > > > Thanks, > > Derek > > _______________________________________________ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
