> On 2011-05-03 17:45:41, Brad Beckmann wrote: > > Can we change the name of Time in base/time.hh instead of Time in Ruby? > > Right now this patch touches 50+ Ruby files and a bunch of lines within > > those files just to change Time to RTime. It seems that far fewer changes > > would be required to change the name of base/time.hh's version of Time. > > Nathan Binkert wrote: > While I wouldn't be opposed to changing the time in base/time.hh if we > needed both, but don't we need to move ruby away from its own version of Time > anyway? Shouldn't we be using Tick? > > Also, we've generally never shied away from changes that can be done with > a one line sed/perl script. > > Gabe Black wrote: > I was about to say something along these lines. This would be a good > chance to consolidate Ruby and M5 systems a little. It might be a good idea > to do the simple find/replace change on its own so the non-simple > non-find/replace stuff doesn't get lost as noise. I don't have deep feelings > one way or the other, though.
This should be two diffs: 1 for rename and 1 for stat code. But since they are related to getting dumpstats working I combined them both for the sake of discussion. Time isnt equivalent Tick or I would've happily made that change :). Time is really the Ruby Cycle count so if you change it to Tick then all over the place you would have to convert to cycles when making comparisons for request latencies and various parameters in Ruby. In the interim, maybe changing Time to Cycle would work. Alternatively, you could change Time to Tick, and then force all the latencies throughout Ruby to be expressed in Ticks. This would have the nice effect of setting the latencies for Caches and memory objects with a real time (e.g. "1ns") instead of relative time (e.g. 1 cycle). > On 2011-05-03 17:45:41, Brad Beckmann wrote: > > src/mem/ruby/system/System.hh, line 93 > > <http://reviews.m5sim.org/r/675/diff/1/?file=12399#file12399line93> > > > > Why does this need to be static? I think we want to avoid making it > > more difficult to run Ruby in a multiple system scenario. The network and tracer objects are already static in the system class, so at the time it made sense to make this static as well, since there will only ever be 1 profiler even in the multiple systems case (even in multiple systems, right?) Also, I cant remember the exact scenario, but there was wierdness with grabbing the Profiler pointer in the schedProfileEvent function, and I believe that was because you didnt have an instance of a Profiler object which can be resolved by making that a static variable. That's worth a revisit. > On 2011-05-03 17:45:41, Brad Beckmann wrote: > > src/mem/ruby/profiler/Profiler.cc, line 751 > > <http://reviews.m5sim.org/r/675/diff/1/?file=12380#file12380line751> > > > > Why do you have to create a separate namespace here? Relic from mimicking the old schedStatEvent function and when I thought that the Profiler was the only place that cared about stats (the Network does too). I suppose placing all the code in RubySystem and then accessing things using "RubySystem::function()" would work just as well. But again, a lot of this stats stuff would be solved if Ruby registered stats similar to the way M5 does. > On 2011-05-03 17:45:41, Brad Beckmann wrote: > > src/sim/pseudo_inst.cc, line 75 > > <http://reviews.m5sim.org/r/675/diff/1/?file=12404#file12404line75> > > > > Why is the RUBY compile flag necessary? If Ruby isn't compiled in, then calling RubyProfile::function wouldnt compile correctly as none of the Ruby code would be recognized. - Korey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/675/#review1195 ----------------------------------------------------------- On 2011-05-03 11:20:58, Korey Sewell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/675/ > ----------------------------------------------------------- > > (Updated 2011-05-03 11:20:58) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ruby-stats: support for dump_stats instruction > *** > NOTE: The core changes for this diff are in Profiler.cc/hh, > stat_control.hh/cc, and pseudo_inst.cc > **** > This is a first pass toward getting dump-stats functionality to work > "cleanly" for Ruby. As is, the patch works, but there needs to be discussion > over: > - Changing Ruby typedef "Time" to "RTime" because it conflicts with the Time > class defined in base/time.hh (The majority of files are renames)... Is there > a better name than "RTime"? > > - Where is the right place for this RubyStatEvent code? I hesitated to do any > real cosmetic changes because of what impending stat changes might do. I have > two thoughts: > (1) If Ruby Stats will be registered like old M5 stats, then this code would > nicely fold into the old "statEvent" code in sim_control.cc. "Fold into" > maybe too strong of a phrase even, as most of it should just naturally "work". > (2) If Ruby Stats are not registered, then maybe placing this code into the > RubySystem class. I realized late that the "Profiler" and "Network" have two > different stats that they track so the RubySystem would be the right place > along with calling the namespace "RubyStats". > > > Diffs > ----- > > SConstruct 3f49ed206f46 > src/cpu/testers/rubytest/RubyTester.hh 3f49ed206f46 > src/cpu/testers/rubytest/RubyTester.cc 3f49ed206f46 > src/mem/ruby/buffers/MessageBuffer.hh 3f49ed206f46 > src/mem/ruby/buffers/MessageBuffer.cc 3f49ed206f46 > src/mem/ruby/buffers/MessageBufferNode.hh 3f49ed206f46 > src/mem/ruby/common/Consumer.hh 3f49ed206f46 > src/mem/ruby/common/Global.hh 3f49ed206f46 > src/mem/ruby/common/TypeDefines.hh 3f49ed206f46 > src/mem/ruby/eventqueue/RubyEventQueue.hh 3f49ed206f46 > src/mem/ruby/eventqueue/RubyEventQueue.cc 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.hh 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/InputUnit_d.hh 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.hh > 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/OutVcState_d.hh 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/VirtualChannel_d.hh 3f49ed206f46 > src/mem/ruby/network/garnet/fixed-pipeline/flit_d.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/FlexibleConsumer.hh > 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.cc 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/InVcState.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/InVcState.cc 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.hh > 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkLink.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkLink.cc 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/OutVcState.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/OutVcState.cc 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/Router.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/Router.cc 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/flit.hh 3f49ed206f46 > src/mem/ruby/network/garnet/flexible-pipeline/flit.cc 3f49ed206f46 > src/mem/ruby/network/simple/Throttle.hh 3f49ed206f46 > src/mem/ruby/profiler/Profiler.hh 3f49ed206f46 > src/mem/ruby/profiler/Profiler.cc 3f49ed206f46 > src/mem/ruby/profiler/StoreTrace.hh 3f49ed206f46 > src/mem/ruby/profiler/StoreTrace.cc 3f49ed206f46 > src/mem/ruby/recorder/CacheRecorder.hh 3f49ed206f46 > src/mem/ruby/recorder/CacheRecorder.cc 3f49ed206f46 > src/mem/ruby/recorder/TraceRecord.hh 3f49ed206f46 > src/mem/ruby/recorder/TraceRecord.cc 3f49ed206f46 > src/mem/ruby/recorder/Tracer.hh 3f49ed206f46 > src/mem/ruby/recorder/Tracer.cc 3f49ed206f46 > src/mem/ruby/slicc_interface/AbstractCacheEntry.hh 3f49ed206f46 > src/mem/ruby/slicc_interface/Message.hh 3f49ed206f46 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh 3f49ed206f46 > src/mem/ruby/system/AbstractReplacementPolicy.hh 3f49ed206f46 > src/mem/ruby/system/LRUPolicy.hh 3f49ed206f46 > src/mem/ruby/system/MemoryControl.cc 3f49ed206f46 > src/mem/ruby/system/MemoryNode.hh 3f49ed206f46 > src/mem/ruby/system/PseudoLRUPolicy.hh 3f49ed206f46 > src/mem/ruby/system/Sequencer.hh 3f49ed206f46 > src/mem/ruby/system/Sequencer.cc 3f49ed206f46 > src/mem/ruby/system/System.hh 3f49ed206f46 > src/mem/ruby/system/System.cc 3f49ed206f46 > src/mem/ruby/system/TimerTable.hh 3f49ed206f46 > src/mem/ruby/system/TimerTable.cc 3f49ed206f46 > src/mem/ruby/system/WireBuffer.cc 3f49ed206f46 > src/sim/pseudo_inst.cc 3f49ed206f46 > src/sim/stat_control.hh 3f49ed206f46 > src/sim/stat_control.cc 3f49ed206f46 > > Diff: http://reviews.m5sim.org/r/675/diff > > > Testing > ------- > > > Thanks, > > Korey > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
