----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/277/#review398 -----------------------------------------------------------
src/cpu/testers/rubytest/CheckTable.cc <http://reviews.m5sim.org/r/277/#comment531> These adjacent DPRINTFs should be consolidated into one. It's more efficient (since it's a single check of the trace flag and a single call to cprintf), and in this case (and many others) we don't really need the function name, file name, and line number printed redundantly. Also you don't need to use c_str(), cprintf() handles std::string just fine (and anything else that supports the '<<' operator, I believe). src/mem/SConscript <http://reviews.m5sim.org/r/277/#comment526> Do we want to define a whole new disjoint set of trace flags just for Ruby? For example, why have RubyCache and Cache both? src/mem/protocol/MESI_CMP_directory-L1cache.sm <http://reviews.m5sim.org/r/277/#comment527> This DPRINTF is broken (and several others like it)... it needs a trace flag as the first arg, and "%str" probably isn't the format string you want (basically it's just like printf, so you probably mean "%s"). src/mem/protocol/MESI_CMP_directory-L1cache.sm <http://reviews.m5sim.org/r/277/#comment528> We should get rid of these commented-out lines, either deleting them if they're not needed or just using a trace flag that keeps them turned off unless wanted. That's the point of the trace flags, is so that you don't have to comment out debug printfs, but can just turn them on and off dynamically at the granularity you want. src/mem/ruby/buffers/MessageBuffer.cc <http://reviews.m5sim.org/r/277/#comment529> Note that DPRINTF automatically prepends the value of name(), so we should make sure that works for Ruby so we don't need to explicitly pass m_name.c_str() (which I assume is the same or similar). src/mem/ruby/common/Address.hh <http://reviews.m5sim.org/r/277/#comment530> Indentation looks funky on this line. Are you using tabs? src/mem/ruby/common/Address.hh <http://reviews.m5sim.org/r/277/#comment525> Nate can correct me if I'm wrong, but the cprintf library eventually uses '<<' at the bottom, so if we need to define a type-specific output (here and with DataBlock below) we should just overload that operator. In this specific case, much of this formatting can be handled in the format string itself as "[0x%x, line 0x%x]" and then calling with addr and blockAlign(addr) as arguments. src/mem/ruby/common/Debug.hh <http://reviews.m5sim.org/r/277/#comment524> We need to decide how valuable it is to automatically include the function name, file, and line number. If there's a consensus that we should keep it, we should add DPRINTF variants to M5 that have this. If not, we should eliminate this info from the Ruby debug messages. But we shouldn't take a bunch of boilerplate that used to be nicely encapsulated in a macro and reproduce it N times all over the code. src/mem/ruby/common/Debug.cc <http://reviews.m5sim.org/r/277/#comment523> Code should never just be commented out... if we don't need it any more, just delete it. - Steve On 2010-10-19 17:30:48, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/277/ > ----------------------------------------------------------- > > (Updated 2010-10-19 17:30:48) > > > Review request for Default and Ruby Reviewers. > > > Summary > ------- > > Ruby currently uses GEMS debug support with the enum character string map to > enable certain debug messages. Meanwhile, M5 has debug print support that > works with scons. Compiling the m5.fast binary, the M5 debug statements are > removed, but the Ruby ones are not unless RUBY_DEBUG is not defined. This > patch moves Ruby to M5's debug print support. > > > Diffs > ----- > > src/cpu/testers/rubytest/CheckTable.cc 956ac83b0a58 > src/mem/SConscript 956ac83b0a58 > src/mem/protocol/MESI_CMP_directory-L1cache.sm 956ac83b0a58 > src/mem/protocol/MESI_CMP_directory-L2cache.sm 956ac83b0a58 > src/mem/protocol/MESI_CMP_directory-dir.sm 956ac83b0a58 > src/mem/protocol/MI_example-cache.sm 956ac83b0a58 > src/mem/protocol/MI_example-dir.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-dir.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-perfectDir.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_token-L1cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_token-L2cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_token-dir.sm 956ac83b0a58 > src/mem/protocol/MOESI_hammer-cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_hammer-dir.sm 956ac83b0a58 > src/mem/protocol/RubySlicc_Exports.sm 956ac83b0a58 > src/mem/protocol/RubySlicc_MemControl.sm 956ac83b0a58 > src/mem/protocol/RubySlicc_Types.sm 956ac83b0a58 > src/mem/ruby/SConsopts 956ac83b0a58 > src/mem/ruby/buffers/MessageBuffer.cc 956ac83b0a58 > src/mem/ruby/common/Address.hh 956ac83b0a58 > src/mem/ruby/common/DataBlock.hh 956ac83b0a58 > src/mem/ruby/common/Debug.hh 956ac83b0a58 > src/mem/ruby/common/Debug.cc 956ac83b0a58 > src/mem/ruby/common/NetDest.hh 956ac83b0a58 > src/mem/ruby/common/NetDest.cc 956ac83b0a58 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 956ac83b0a58 > src/mem/ruby/network/garnet/fixed-pipeline/Switch_d.cc 956ac83b0a58 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 956ac83b0a58 > src/mem/ruby/network/garnet/flexible-pipeline/Router.cc 956ac83b0a58 > src/mem/ruby/network/simple/PerfectSwitch.cc 956ac83b0a58 > src/mem/ruby/network/simple/Throttle.cc 956ac83b0a58 > src/mem/ruby/network/simple/Topology.cc 956ac83b0a58 > src/mem/ruby/slicc_interface/Message.hh 956ac83b0a58 > src/mem/ruby/slicc_interface/NetworkMessage.hh 956ac83b0a58 > src/mem/ruby/storebuffer/storebuffer.cc 956ac83b0a58 > src/mem/ruby/system/CacheMemory.cc 956ac83b0a58 > src/mem/ruby/system/DirectoryMemory.cc 956ac83b0a58 > src/mem/ruby/system/SConscript 956ac83b0a58 > src/mem/ruby/system/SparseMemory.cc 956ac83b0a58 > src/mem/ruby/tester/RaceyPseudoThread.cc 956ac83b0a58 > src/mem/slicc/ast/FuncCallExprAST.py 956ac83b0a58 > src/mem/slicc/symbols/StateMachine.py 956ac83b0a58 > src/mem/slicc/symbols/Type.py 956ac83b0a58 > > Diff: http://reviews.m5sim.org/r/277/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
